Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor some code #28

Merged
merged 9 commits into from
Jul 3, 2016
Merged

Refactor some code #28

merged 9 commits into from
Jul 3, 2016

Conversation

lokeshh
Copy link
Member

@lokeshh lokeshh commented May 17, 2016

I had a look at some places where code can be improved while documenting the functions. This PR is for that purpose.

.new(@data_set, @dependent, @opts)
@regression = ['Statsample', 'GLM', algorithm, method]
.reduce(Module, :const_get)
.new @data_set, @dependent, @opts
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@v0dro I see your comment to remove const_get. Is there a way to do it without using const_get?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the need of chaining basically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the current change fine or should I change to something like Object.const_get('Daru::Vector')?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep a single const_get. In other words go with the SO answer.

@lokeshh
Copy link
Member Author

lokeshh commented May 19, 2016

Changed degree_of_freedom to degrees_of_freedom and standard_error to standard_errors. Also added aliases so the old names could be used. I find the tests for these functions are missing.

Should I add the tests? Also should I print a error message to tell that the old names will be removed?

@v0dro
Copy link
Member

v0dro commented May 20, 2016

Aren't these the tests you're looking for? Yes print an error message into $stderr.

@agisga
Copy link

agisga commented May 20, 2016

I saw too that not every method is tested for every model (logistic, probit, normal etc.), and in general there is a lot of room for improvement of the tests. For example #standard_errors is only tested for logistic regression, and I think you are right that #degrees_of_freedom is never tested.
Though maybe addition of missing tests should be a separate commit (to make the commit history more organized)?

@v0dro
Copy link
Member

v0dro commented May 22, 2016

Ah yes there's no tests but those for logit. And new tests should be a separate commit.

@lokeshh
Copy link
Member Author

lokeshh commented Jul 1, 2016

I think this is ready to be merged now. Here are the changes:

  • Replace multiple const_getwith single const_get.
  • Deprecate degree_of_freedom and introduce degrees_of_freedom
  • Deprecate standard_error and introduce standard_errors

@v0dro v0dro merged commit 79b5f48 into SciRuby:master Jul 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants