Skip to content

[ENH] tests for TiDE Model #1843

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

Merged
merged 37 commits into from
May 29, 2025
Merged

Conversation

PranavBhatP
Copy link
Contributor

Description

This PR fixes #1807 and stacks upon PR #1780. Builds upon the PR #1814 (closed due to complex commit history).

Checklist

  • Linked issues (if existing)
  • Amended changelog for large changes (and added myself there as contributor)
  • 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

fkiraly and others added 30 commits February 22, 2025 23:18
### Description

This PR implements the basic skeleton of D1 and D2 layer for v2.

See sktime#1736 for discussion and design.
This PR solves the bug in `data_module` where the
`static_categorical_features` and `static_continuous_features` were not
correctly calculated in `__getitem__` of nested class
…1832)

This PR makes the `data_modulel` dataclass-like
See discussion in sktime#1829
Copy link

codecov bot commented May 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@aa3bc9d). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1843   +/-   ##
=======================================
  Coverage        ?   85.01%           
=======================================
  Files           ?       62           
  Lines           ?     6166           
  Branches        ?        0           
=======================================
  Hits            ?     5242           
  Misses          ?      924           
  Partials        ?        0           
Flag Coverage Δ
cpu 85.01% <100.00%> (?)
pytest 85.01% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

There are also tests for TFT in here - does this PR accidentally stack on something else as well?

@PranavBhatP
Copy link
Contributor Author

PranavBhatP commented May 23, 2025

There are also tests for TFT in here - does this PR accidentally stack on something else as well?

I had pulled the latest changes. Fixed it now with a rebase.

@PranavBhatP PranavBhatP requested a review from fkiraly May 23, 2025 07:19
@phoeenniixx
Copy link
Contributor

Fixed it now with a rebase.

Not sure what happened, but just speaking from experience, I would suggest to prefer merge over rebase. Sometimes it creates more hassle than it’s worth.
See this comment: sktime/sktime#7334 (comment)

@PranavBhatP
Copy link
Contributor Author

I would suggest to prefer merge over rebase. Sometimes it creates more hassle than it’s worth.

Thanks for the heads-up. For now, the commit history seems fine, if any issues do come up, then the issue might point to this.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Regarding your questions what to do next:

  • I would add the metadata class next, simply follow pattern in DeepARMetadata, and see what happens if it gets tested
  • potentially, there is a clash between the integrations, in this case we need to resolve by adding further variable parts
  • @phoeenniixx is not stacking on 1843 for v1, so you can add a metadata class and you do not need to stack on each other (he is diverging)

@PranavBhatP
Copy link
Contributor Author

PranavBhatP commented May 27, 2025

@fkiraly

I would add the metadata class next, simply follow pattern in DeepARMetadata, and see what happens if it gets tested

Yes, this change has been pushed, for now I have returned only one-two set of arguments in get_test_train_params(). We can extend the number of test kwargs later.

potentially, there is a clash between the integrations, in this case we need to resolve by adding further variable parts

I ran the test_all_estimators.py file on TiDEModel, and there are errors due to model-specific implementations in TiDE. Will try figuring out changes to make this work. Until then this PR is blocked. You can view the errors once the CI runs completes.

EDIT: these were small fixes, everything seems to be working fine now.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

ok, I will merge #1780 since two Pr are now stacking or forking. We can add tests for more models later.

@fkiraly fkiraly changed the title [ENH] add tests for TiDE Model. [ENH] tests for TiDE Model. May 27, 2025
@fkiraly fkiraly moved this from PR in progress to PR under review in May - Sep 2025 mentee projects May 28, 2025
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

I merged the test PR to reduce the stack.

Question: you mentioned on #1780 that _integration would pose a problem due to the cell_type arg, but this PR did pass even before I moved it to kwargs.

Can you explain that? Why was it not a problem?

@PranavBhatP
Copy link
Contributor Author

PranavBhatP commented May 28, 2025

I had already made the necessary change in this PR, before you added them to the kwargs in #1780.

@fkiraly
Copy link
Collaborator

fkiraly commented May 28, 2025

I had already made the necessary change in this PR

Ah, I did not see that - what did you do, if I may ask? (I could dig into the commits, but I would appreciate if you summarize, that might be quicker)

Did we make identical changes, or did you do sth else?

In any case, that is what I meant with "identifying variable parts" - for instance, by considering a second example, we now learnt that even the presence of cell_type is variable.

@PranavBhatP
Copy link
Contributor Author

PranavBhatP commented May 28, 2025

what did you do, if I may ask? (I could dig into the commits, but I would appreciate if you summarize, that might be quicker)

I removed cell_type and n_plotting_samples from the from_dataset() method inside test_all_estimators.py. I'm assuming you have made the same changes, hence the absence of a merge conflict?

you can view the changes in the commit.

@fkiraly
Copy link
Collaborator

fkiraly commented May 29, 2025

I removed cell_type and n_plotting_samples from the from_dataset() method inside test_all_estimators.py. I'm assuming you have made the same changes, hence the absence of a merge conflict?

I also added them to the get_test_train_params, so it does not change internal logic, only structure - as a pure refactor.

I am guessing you did not do that? It is not good style to change the logic in another part of the code base (at least without making this clear in the PR description) just to make your changes work.

@fkiraly fkiraly changed the title [ENH] tests for TiDE Model. [ENH] tests for TiDE Model May 29, 2025
@fkiraly fkiraly merged commit 756f61a into sktime:main May 29, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from PR under review to Done in May - Sep 2025 mentee projects May 29, 2025
@PranavBhatP
Copy link
Contributor Author

I am guessing you did not do that? It is not good style to change the logic in another part of the code base (at least without making this clear in the PR description) just to make your changes work.

Will keep this in mind next time. Thanks for pointing it out.

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

Successfully merging this pull request may close these issues.

[ENH] add tests for TiDE in pytorch-forecasting
4 participants