-
Notifications
You must be signed in to change notification settings - Fork 257
[BUG] Initialize _metrics in build_model for all Deep Learning estimators #3198
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
[BUG] Initialize _metrics in build_model for all Deep Learning estimators #3198
Conversation
Thank you for contributing to
|
|
That was quick. Wouldn't it be better to address this issue for all deep learning-based estimators at once? The ones that have the same problem with $ rg "build_model" -l
aeon/testing/mock_estimators/_mock_clusterers.py
aeon/regression/deep_learning/tests/test_deep_regressor_base.py
aeon/clustering/deep_learning/_ae_drnn.py
aeon/regression/deep_learning/_inception_time.py
aeon/regression/deep_learning/_mlp.py
aeon/regression/deep_learning/_encoder.py
aeon/regression/deep_learning/_disjoint_cnn.py
aeon/clustering/deep_learning/_ae_resnet.py
aeon/regression/deep_learning/_cnn.py
aeon/regression/deep_learning/base.py
aeon/regression/deep_learning/_resnet.py
aeon/regression/deep_learning/_fcn.py
aeon/regression/deep_learning/_rnn.py
aeon/clustering/deep_learning/_ae_fcn.py
aeon/regression/deep_learning/_lite_time.py
aeon/clustering/deep_learning/_ae_bgru.py
aeon/classification/deep_learning/_resnet.py
aeon/clustering/deep_learning/base.py
aeon/clustering/deep_learning/_ae_dcnn.py
aeon/clustering/deep_learning/_ae_abgru.py
aeon/classification/deep_learning/_lite_time.py
aeon/classification/deep_learning/_mlp.py
aeon/classification/deep_learning/tests/test_deep_classifier_base.py
aeon/classification/deep_learning/_disjoint_cnn.py
aeon/classification/deep_learning/base.py
aeon/classification/deep_learning/_inception_time.py
aeon/classification/deep_learning/_encoder.py
aeon/classification/deep_learning/_fcn.py
aeon/classification/deep_learning/_cnn.py
aeon/transformations/collection/self_supervised/_trilite.py
aeon/transformations/collection/self_supervised/_time_mcl.py
aeon/forecasting/deep_learning/base.py
aeon/forecasting/deep_learning/_dummy_series_forecaster.py
aeon/forecasting/deep_learning/_deepar.py
aeon/forecasting/deep_learning/_tcn.py
aeon/forecasting/deep_learning/tests/test_base.py |
|
Agreed, that makes complete sense @jsquaredosquared . Thanks for the list! I will scan through those estimators and apply the fix to all of them that share this initialization pattern. I'll update the PR to cover the full scope. |
|
Hi @satwiksps , consider changing the title of the PR to reflect the broadened scope. Also, would it make more sense to just move the setting of |
|
Your suggestion is cleaner and architecturally correct than my conservative fix. That makes a lot of sense @jsquaredosquared. I'll refactor the changes to remove the duplication from _fit |
|
This looks much cleaner. I can add a small regression test to ensure build_model works without calling fit first, if that’s helpful. @satwiksps |
Hello @shubhamshukla07
After reviewing this PR, if the maintainers suggest, I will propose my local testing code which I used, to include in the test suite permanently, if the maintainers are not satisfied with the testing code, you may add any test you feel right after this PR gets merged |
|
Hello @hadifawaz1999 I've done some refactoring in the implementation. I moved |
The previous CI run failed in 'aeon/datasets/tests/test_rehabpile_loader.py' due to an OSError (read operation timed out) while attempting to download the RehabPile dataset from an external source. This failure is unrelated to the changes in this PR. Pushing this empty commit to re-trigger the workflow.
hadifawaz1999
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.
Thanks for this. LGTM
Reference Issues/PRs
What does this implement/fix? Explain your changes.
This PR fixes a bug where
build_modelwould raise anAttributeErrorif called before the model had been fitted. This issue affectedResNetClassifierand approximately 25 other deep learning estimators across the library.The root cause was that
self._metricswas only being initialized inside the_fitmethod. Thebuild_modelmethod attempts to compile the model usingself._metrics, causing a crash if the user attempted to build the Keras model directly (e.g., for custom training loops) without callingfitfirst.Changes:
build_modelfor all affected deep learning estimators (Classification, Regression, Clustering, and Forecasting).self._metricsdoes not exist, it is now lazily initialized fromself.metricsusing the same logic found in_fit(including handling ofNonefor clustering models).self.metricsisNone, ensuring it defaults correctly (typically to["mean_squared_error"]) just like in_fitI verified the fix locally by writing a script that attempts to instantiate and build every affected model without fitting. All 25 models now pass successfully.
Click to view verification script
Verification Script:
Click to view output of verification script
Output:
Does your contribution introduce a new dependency? If yes, which one?
No.
Any other comments?
No.
PR checklist
For all contributions
For new estimators and functions
__maintainer__at the top of relevant files and want to be contacted regarding its maintenance. Unmaintained files may be removed. This is for the full file, and you should not add yourself if you are just making minor changes or do not want to help maintain its contents.For developers with write access