-
Notifications
You must be signed in to change notification settings - Fork 68
Refactor code in preparation for voting learners #622
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
Conversation
- This is useful since it can the be used by both regular learners as well as the voting learners.
- Otherwise things break on Windows.
|
Hello @desilinguist! Thanks for updating this PR.
Comment last updated at 2020-07-01 16:43:16 UTC |
|
Not sure why it doesn't show here but the Travis builds are actually complete. |
|
Actually, this is not done - just found another simplification I can make. Please don't review it yet. |
Codecov Report
@@ Coverage Diff @@
## master #622 +/- ##
==========================================
- Coverage 95.15% 95.10% -0.05%
==========================================
Files 26 27 +1
Lines 3031 3083 +52
==========================================
+ Hits 2884 2932 +48
- Misses 147 151 +4
Continue to review full report at Codecov.
|
|
@bndgyawali @mulhod @aoifecahill this is now ready for review. The minor code coverage decrease is expected due to the refactoring. |
mulhod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I love the restructuring. It will make development easier when the source files are shorter.
I had a few very minor, nitpicky suggestions for docstrings, etc.
aoifecahill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
Co-authored-by: Matt Mulholland <mulhodm@gmail.com>
As I was working on implementing a
VotingLearnerclass (for #488), I realized that this implementation would end up sharing a lot of the code with methods for theLearnerclass. One way to handle this is to makeVotingLearnerinherit fromLearnerbut that's not really right because, in fact,VotingLearneruses multiple underlyingLearnerinstances. So, the solution I came up with is to refactor as much of this shared code into functions inlearner.utilsthat can the be used by bothLearnerinstances as well asVotingLearnerinstances (and, hopefully,StackingLearnerinstances too, when we get to those).Specifically, this PR refactors:
Learner.evaluate()intolearner.utils.compute_evaluation_metrics().Learner.predict()intolearner.utils.write_predictions().Learner.evaluate()intolearner.utils.add_unseen_labels().Learner._compute_num_folds_from_example_counts()intolearner.utils.compute_num_folds_from_example_counts().Learner.predict()intolearner.utils.get_predictions().Learner.cross_validate()intolearner.utils.setup_cv_fold_iterator().Learner.learning_curve()intolearner.utils.setup_cv_split_iterator().In addition, This PR closes #621 and adds a test to make sure that the predictions being returned and written out via
Learner.predict()match expectations. As part of this fix, the default value of theclass_labelskeyword argument forLearner.predict()is now set toTrueinstead ofFalsesince it doesn't make sense to return class indices by default.💥 This will be a an API-breaking change since to get class probabilities as outputs,
class_labelswill explicitly need to be set toFalse. 💥Finally, this PR improves many docstrings, replaces single quotes with double quotes in some places, and replaces the old-style
formatstrings with new-style ones in some of the code that was touched.