-
Notifications
You must be signed in to change notification settings - Fork 905
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
Add Model objects to forecasting #1558
Add Model objects to forecasting #1558
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.
Thanks for creating the PR! I will plan to do a full review hopefully next week after v0.4.0 releases.
6300a85
to
a82ad69
Compare
Job PR-1558-3 is done. |
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 Caner! It looks good - we can always merge this in first and then add support for mean
after. Great job on the thorough tests too.
forecasting/src/autogluon/forecasting/models/abstract/abstract_forecasting_model.py
Show resolved
Hide resolved
forecasting/src/autogluon/forecasting/models/abstract/abstract_forecasting_model.py
Outdated
Show resolved
Hide resolved
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 very good overall!
forecasting/src/autogluon/forecasting/models/gluonts/abstract_gluonts.py
Outdated
Show resolved
Hide resolved
forecasting/src/autogluon/forecasting/models/gluonts/abstract_gluonts.py
Show resolved
Hide resolved
forecasting/src/autogluon/forecasting/models/gluonts/abstract_gluonts.py
Outdated
Show resolved
Hide resolved
forecasting/src/autogluon/forecasting/models/gluonts/abstract_gluonts.py
Show resolved
Hide resolved
forecasting/src/autogluon/forecasting/models/gluonts/abstract_gluonts.py
Show resolved
Hide resolved
forecasting/src/autogluon/forecasting/models/gluonts/abstract_gluonts.py
Show resolved
Hide resolved
a82ad69
to
8fb8d46
Compare
In the second revision of the PR, we've
|
cc @huibinshen |
@Innixma I can't tell why CI wasn't triggered. |
@canerturkmen Looked like a transient CI error, I retriggered CI, hopefully it passes. |
Job PR-1558-7 is done. |
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! Awesome contribution. A few minor things can be refined in future PRs once we align on how this integrates with Trainer, Learner, and Predictor.
The class object of the GluonTS estimator to be used. | ||
""" | ||
|
||
def __init__(self, gluonts_estimator_class: Type[GluonTSEstimator], **kwargs): |
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.
Should gluonts_estimator_class
instead be a hyperparameter? How would the user specify a particular GluonTS model to train alongside others via Predictor
?
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 see the need for this and will consider it in a future PR. Thanks!
) | ||
|
||
|
||
# TODO: AutoGluon Tabular will be removed from GluonTS to avoid circular dependencies |
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.
Another option is that GluonTS can continue to depend on AutoGluon as an optional and lazily imported dependency that we explicitly don't use / import in AutoGluon Forecasting.
Meaning we would have AutoTabularModel implemented separately in both AG and GluonTS.
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.
Another (temporary) hack may be if GluonTS just depends on autogluon.tabular sub-package instead of the full autogluon.
Long term though, the AutoTabularModel should probably be moved back into just AG as it's not a core part of GluonTS.
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.
GluonTS does not have autogluon as a core dependency, but only as an optional requirement for users who want to use AG Tabular; so I think we are safe in the near future.
Issue #, if available:
Earlier PR: #1538
Description of changes:
This PR adds model classes to
autogluon.forecasting
, porting over previous work and significantly refactoring:AbstractForecastingModel
, partially inheriting fromcore
'sAbstractModel
to modularize the code and remove code dupes. Most of the remaining code dupe is in themodel_trial
module, however these had to be re-implemented because of the "supervised model" signature (X, y
) among other reasons.AbstractGluonTSModel
.Coverage report:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.