-
Notifications
You must be signed in to change notification settings - Fork 80
Poc: base classes for deep models and rnn and deepstate with examples #776
Conversation
🚀 Deployed on https://deploy-preview-776--etna-docs.netlify.app |
Codecov Report
@@ Coverage Diff @@
## master #776 +/- ##
===========================================
- Coverage 83.69% 49.52% -34.17%
===========================================
Files 124 125 +1
Lines 6959 7193 +234
===========================================
- Hits 5824 3562 -2262
- Misses 1135 3631 +2496
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
etna/models/base.py
Outdated
pass | ||
|
||
|
||
class DeepBaseModel(LightningModule, FitAbstractModel, DeepBaseAbstractModel, BaseMixin): |
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 need inheritance from LightningModule
here? I thought that in general composition is better than inheritance.
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.
general composition is better than inheritance.
I don't think that passing LightningModule as parameter make any difference in our case because we don't have any coupled logic and deep hierarchy. But we will write a little bit extra code for wrappers with duplicated docs.
What downsides do you think of about?
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.
Am I correctly understand that you decided to make dependency injection here?
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'll try
Notebook seems to be outdated |
a6c01e9
to
c8df114
Compare
Improve description in the notebook:
|
I guess we need some more tests:
|
e86704b
to
8c83571
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.
Almost done)
There are some unresolved threads left
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 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.
Waiting for some "little" changes)
Before submitting (must do checklist)
Proposed Changes
Closing issues