Conversation
There was a problem hiding this comment.
Thank you and welcome to the community @Shvrth !
This looks good!
I have some suggestions:
- Use numpydoc style for docstrings.
- Add the layers to
layersmodule - utils to
utilsmodule maybe? - Use
_safe_importto import soft dep used here. As I could seeeinopsis not a core dep here inptf. Seepyproject.tomlto see the core deps (here) - Add a
_pkgclass 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.
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.
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.
This should be added to layers/attention?
There was a problem hiding this comment.
do we not already have an attention layer?
There was a problem hiding this comment.
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.
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.
Yes, thank you for the feedback. I'll update the code to include this.
There was a problem hiding this comment.
@PranavBhatP, would the current test suite not already run the _integration, with parameters from the pkg class?
There was a problem hiding this comment.
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.
|
@Shvrth are you still working on this? I see the tests are failing. If you are facing any issues, please let us know! |
4 participants
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