-
Notifications
You must be signed in to change notification settings - Fork 705
[ENH] Duet Implementation #1968
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
base: main
Are you sure you want to change the base?
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.
Thank you and welcome to the community @Shvrth !
This looks good!
I have some suggestions:
- Use numpydoc style for docstrings.
- Add the layers to
layers
module - utils to
utils
module maybe? - Use
_safe_import
to import soft dep used here. As I could seeeinops
is not a core dep here inptf
. Seepyproject.toml
to see the core deps (here) - Add a
_pkg
class for testing. See other models to see how it is written! Some examples: xlstm, deepar, timexer
from logging import config | ||
from typing import Callable, Optional, Union | ||
|
||
from einops import rearrange |
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 we use _safe_import
here, this is not a core dep of ptf
, so it should be imported using _safe_import
. See here
""" | ||
Initialize DUET model. | ||
|
||
Args: |
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.
Can you add these to pytorch-forecasting/layers
module, rather than in the duet
folder? There you may find some layers that you could reuse here? Like revin
?
return V.contiguous(), None | ||
|
||
|
||
class AttentionLayer(nn.Module): |
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.
This should be added to layers/attention
?
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.
do we not already have an attention layer?
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.
Maybe you should add the utils to utils
module and layers that are present here should go to layers
?
print("--------------------------------------\n") | ||
|
||
|
||
def configure_dataset() -> tuple[TimeSeriesDataSet, TimeSeriesDataSet]: |
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 have a change request w.r.t tests for duet. Could you try creating an _integration
function to run the entire pipeline starting from the dataset creation all the way to the predictions. You can refer to this example - test_timexer.
You can then run individual tests on DUET
by calling this _integration
on different kinds of data. You can understand how to do this from the linked example.
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.
Yes, thank you for the feedback. I'll update the code to include this.
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.
@PranavBhatP, would the current test suite not already run the _integration
, with parameters from the pkg class?
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.
that also works, but since @Shvrth has already included a test file for duet, I suggested the change since we have a common pattern of testing these models in their respective files. Maybe there are also model specific cases to be tested.
Reference Issues/PRs
Fixes #1938
What does this implement/fix? Explain your changes.
This is to integrate the DUET model with pytorch-forecasting as per issue #1938
Link to paper: https://arxiv.org/abs/2412.10859
Link to official implementation: https://github.com/decisionintelligence/DUET
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
I have added a test script for verifying the model's training process at tests/test_models/test_duet.py
Any other comments?
The current implementation does not yet include the time-based features from the original DUET model. This will be addressed in a future commit.
PR checklist
pre-commit install
.To run hooks independent of commit, execute
pre-commit run --all-files