-
Notifications
You must be signed in to change notification settings - Fork 417
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
Optionally Avoid recomputing features #722
Conversation
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.
exclude_states
should also be added to:
MetricCollection
MinMaxMetric
MultioutputWrapper
MetricTracker
for consistency
3a0f7dc
to
317182c
Compare
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
@justusschock still draft or ready to read? 🐰 |
for more information, see https://pre-commit.ci
is it related to #709? 🐰 |
what kind of feedback, could you please remind me of the thread... 🐰 |
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.
To avoid bug when passing None
@justusschock instead of making this PR have influence on the base class, what about re-implementing just def reset(self):
if not self.reset_real_features:
# remove temporarily to avoid resetting
value = self._defaults.pop('real_features')
super().reset()
self._defaults['real_features'] = value
else:
super().reset() this feature seems to only be relevant to |
@SkafteNicki works for me. However, afaik there are some similar metrics in NLP which might be added some day. This is why I aimed for a more general approach here. I'm fine with both, just let me know your preference :) |
@justusschock I would go with the more local solution for the affected metrics for now. If we then in the future have other metrics that need a similar feature then I am fine with revisiting this :] |
Codecov Report
@@ Coverage Diff @@
## master #722 +/- ##
======================================
- Coverage 95% 83% -12%
======================================
Files 173 173
Lines 7381 7401 +20
======================================
- Hits 7012 6149 -863
- Misses 369 1252 +883 |
@justusschock took the liberty to update the PR, please take a look :] |
@SkafteNicki Thanks for updating. Seems you were faster :) LGTM :) |
What does this PR do?
Optionally avoids recomputing the features.
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃