-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fixed set_period()
not updating decomposer parameters
#3932
Conversation
set_period
not updating in decomposer parameters
set_period
not updating in decomposer parametersset_period()
not updating decomposer parameters
Codecov Report
@@ Coverage Diff @@
## main #3932 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 346 346
Lines 36709 36727 +18
=======================================
+ Hits 36576 36594 +18
Misses 133 133
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
d68cab9
to
55743b7
Compare
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.
LGTM but would be nice to have an explicit test for the decomposer.
@@ -205,7 +205,7 @@ def set_period(self, X: pd.DataFrame, y: pd.Series): | |||
|
|||
""" | |||
self.period = self.determine_periodicity(X, y) | |||
self.parameters.update({"period": self.period}) | |||
self.update_parameters({"period": self.period}) |
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.
can you add a test verifying the period updates?
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.
There's a line in test_decomposer.py::test_decomposer_set_period
that's currently commented out testing this, you should be able to uncomment that!
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 uncommented that line, I think that should be sufficient? @jeremyliweishih I can also break it out into its own separate test if need be.
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.
@christopherbunn thats great!
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! I agree with Jeremy about the testing. Have you verified if there's anywhere else we update parameters that could benefit from this function?
@@ -205,7 +205,7 @@ def set_period(self, X: pd.DataFrame, y: pd.Series): | |||
|
|||
""" | |||
self.period = self.determine_periodicity(X, y) | |||
self.parameters.update({"period": self.period}) | |||
self.update_parameters({"period": self.period}) |
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.
There's a line in test_decomposer.py::test_decomposer_set_period
that's currently commented out testing this, you should be able to uncomment that!
@eccabay I did a quick ctrl+f search for places that update parameters and this should be it. I think this is the only place where we're updating the parameters from inside the component itself. |
Also introduces a way to update parameters in a component.
Resolves #3771