Skip to content

[ENH] EXPERIMENTAL PR: make the data_module dataclass-like #1832

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 4 commits into from
May 18, 2025

Conversation

phoeenniixx
Copy link
Contributor

This PR makes the data_modulel dataclass-like
See discussion in #1829

@phoeenniixx phoeenniixx marked this pull request as draft May 13, 2025 15:34
@phoeenniixx
Copy link
Contributor Author

Hi @fkiraly, I tried to change the tests, is this what is intended? or should I need to make some more changes?

Copy link

codecov bot commented May 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1832   +/-   ##
=======================================
  Coverage        ?   86.77%           
=======================================
  Files           ?       51           
  Lines           ?     5663           
  Branches        ?        0           
=======================================
  Hits            ?     4914           
  Misses          ?      749           
  Partials        ?        0           
Flag Coverage Δ
cpu 86.77% <ø> (?)
pytest 86.77% <ø> (?)

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.

@fkiraly
Copy link
Collaborator

fkiraly commented May 13, 2025

Hi @fkiraly, I tried to change the tests, is this what is intended? or should I need to make some more changes?

Yes, this may be sufficient - but are you sure we do not use the attributes in #1812?

@phoeenniixx
Copy link
Contributor Author

phoeenniixx commented May 13, 2025

Hi @fkiraly, I tried to change the tests, is this what is intended? or should I need to make some more changes?

Yes, this may be sufficient - but are you sure we do not use the attributes in #1812?

we can merge this branch to #1812, then i think this should work?, just few adaptations may be required and most importantly the attributes are used with the help of metadata and __getitem__ there, which use the private variables only, so ig this is safe

And we are mainly using max_encoder_length and max_prediction_length which are not changed here

@phoeenniixx
Copy link
Contributor Author

So should we merge this @fkiraly ?

@fkiraly
Copy link
Collaborator

fkiraly commented May 16, 2025

So should we merge this @fkiraly ?

If this should be merged, you should turn it to a "ready for review" pull request, it is still a draft

@phoeenniixx phoeenniixx marked this pull request as ready for review May 17, 2025 02:48
@phoeenniixx
Copy link
Contributor Author

yes, sorry didn't notice it was draft, i have made the final changes, please review

@fkiraly fkiraly merged commit 4c30351 into sktime:main May 18, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from PR in progress to Done in May - Sep 2025 mentee projects May 18, 2025
@phoeenniixx phoeenniixx deleted the dataclass branch May 18, 2025 08:18
PranavBhatP pushed a commit to PranavBhatP/pytorch-forecasting that referenced this pull request May 22, 2025
…1832)

This PR makes the `data_modulel` dataclass-like
See discussion in sktime#1829
PranavBhatP pushed a commit to PranavBhatP/pytorch-forecasting that referenced this pull request May 22, 2025
…1832)

This PR makes the `data_modulel` dataclass-like
See discussion in sktime#1829
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.

2 participants