Skip to content

Conversation

Shvrth
Copy link

@Shvrth Shvrth commented Sep 8, 2025

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

  • pytorch_forecasting/models/duet contains the model code, and a dataset for testing.
  • tests/test_models/test_duet.py contains a simple test script for training the model.

What should a reviewer concentrate their feedback on?

  • I'm particularly looking for feedback on the implementation of DUET as it's still a work-in-progress.
  • Any suggestions for improving clarity or adhering to the project's style.
  • I've added a test script for the model, but I'm observing that the training loss stagnates after the first epoch. I'm investigating whether this is a general issue (e.g., needing a learning rate scheduler) or a specific flaw in the DUET architecture. Any insights here would be particularly helpful.

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

  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
  • Added/modified tests
  • Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with pre-commit install.
    To run hooks independent of commit, execute pre-commit run --all-files

Copy link
Member

@phoeenniixx phoeenniixx left a 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 see einops is not a core dep here in ptf. See pyproject.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
Copy link
Member

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use numpydoc style for docstrings. See here for example.

Copy link
Member

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):
Copy link
Member

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?

Copy link
Collaborator

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't read the code inside, but I am assuming it may be a slightly different implementation, if yes, we should add it otherwise we could use the existing one. It depends on @Shvrth's requirements...

Copy link
Member

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]:
Copy link
Contributor

@PranavBhatP PranavBhatP Sep 9, 2025

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.

Copy link
Author

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.

Copy link
Collaborator

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?

Copy link
Contributor

@PranavBhatP PranavBhatP Sep 11, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Implementation of DUET
4 participants