-
Notifications
You must be signed in to change notification settings - Fork 15
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
Take care of the long tail of attributes in init #157
Conversation
987610a
to
838770f
Compare
@@ -170,6 +172,9 @@ def fit( | |||
logger.info( | |||
"Fitting utility function finished. Start tuning threshold." | |||
) | |||
self.threshold_ = self._tune_threshold( |
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.
I think it was a bug that this was previously missing. Without this line, the threshold would always be 0.5
as set in __init__
. The bug was revealed when I removed this default value from __init__
. @kiudee am I right here?
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.
(All the other classes follow the pattern of using _tune_threshold
here.)
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.
This is probably easier to see after #155 is done and the diff is cleaned up a bit.
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.
Yes, this seems to be a bug.
838770f
to
0f57c22
Compare
Rebased with #155 in master. |
c13b58f
to
456905a
Compare
This PR is ready for review. There are two unresolved questions that I have commented on inline. After this is done, there are just a handful of "no parameters in init" failures remaining:
All of these have the same cause: The I plan to fix that by renaming all our |
456905a
to
ba27c64
Compare
Rebased to test the new CI. Still ready for review / feedback. |
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 👍
@@ -170,6 +172,9 @@ def fit( | |||
logger.info( | |||
"Fitting utility function finished. Start tuning threshold." | |||
) | |||
self.threshold_ = self._tune_threshold( |
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.
Yes, this seems to be a bug.
Most of these attributes are actually initialized during fit and the values do not need to be set in init. To conform the the scikit-learn estimator API, we should not set them in init and postfix values that are learned from the data with an underscore. This takes care of most of the remaining attributes in init. The remaining attributes are due to `construct_model` being called in `__init__`, which cannot currently be easily fixed. A workaround for that will follow.
ba27c64
to
ced7673
Compare
The two threads are now resolved. I only changed the way the |
Description
After removing some big sets (#154, #155), this PR is supposed to take care of the "long tail" of failures of the
check_no_attributes_set_in_init
sklearn estimator check.This is a work in progress. Builds on #154, #155 which should be reviewed first.Also see the commit message:
Most of these attributes are actually initialized during fit and the
values do not need to be set in init. To conform the the scikit-learn
estimator API, we should not set them in init and postfix values that
are learned from the data with an underscore.
This takes care of most of the remaining attributes in init. The
remaining attributes are due to
construct_model
being called in__init__
, which cannot currently be easily fixed. A workaround forthat will follow.
Motivation and Context
Trying to meet the scikit-learn estimator requirements.
How Has This Been Tested?
Work in progress.
Does this close/impact existing issues?
Related to #94, #116.
Types of changes
Checklist: