-
Notifications
You must be signed in to change notification settings - Fork 80
Enabling passing context into Model.forecast v2 #888
Conversation
🚀 Deployed on https://deploy-preview-888--etna-docs.netlify.app |
Codecov Report
@@ Coverage Diff @@
## inference #888 +/- ##
============================================
Coverage ? 49.58%
============================================
Files ? 130
Lines ? 7501
Branches ? 0
============================================
Hits ? 3719
Misses ? 3782
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
We also need tests for the correct dispatching in pipelines
self.model = cast(ContextRequiredModelType, self.model) | ||
if isinstance(self.model, DeepBaseModel): | ||
current_ts_forecast = current_ts.make_future( | ||
future_steps=self.model.decoder_length, tail_steps=self.model.encoder_length |
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.
self.model.context_size
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.
For DeepBaseModel
they are the same.
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.
Why do we use extra branch in this case?
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.
Because of self.model.decoder_length
.
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.
Ok, we should fix it in the future
|
||
if isinstance(self.model, get_args(ContextRequiredModelType)): | ||
self.model = cast(ContextRequiredModelType, self.model) | ||
if isinstance(self.model, DeepBaseModel): |
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.
Nested ifs are not readable. Better to use plain if cases.
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.
Fixed
Before submitting (must do checklist)
Proposed Changes
Closing issues
Closes #845.