-
Notifications
You must be signed in to change notification settings - Fork 683
[BUG] Nhits weight argument fix in TimeSeriesDataSet #1432
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
NhiTS covariate fix
NHiTS covariate fix
NhiTS covariate Change
NHitS Covariate fix unit test
NHiTS covariate change test
Remove Additional property line
NhiTS Fix
NhiTS Fix
…into Nhits_weight_support This is to add a support for weight argument in case of NhiTS model
Any update? |
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.
We should add a test that ensures the bug is fixed.
The changes to conftest
also seem to override an already existing fixture - is that intentional?
FYI @jobs-git, this PR is 2 years old, feel free to finalize it (branch off the PR)
tests/test_models/conftest.py
Outdated
@@ -199,6 +199,50 @@ def dataloaders_with_different_encoder_decoder_length(data_with_covariates): | |||
) | |||
|
|||
|
|||
@pytest.fixture(scope="session") |
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 is just a copy-paste duplication so I will remove it.
I am going through the issues and realize no one has posted a full example that we could use in testing. Could one of you post a full MRE https://stackoverflow.com/help/minimal-reproducible-example @manitadayon, @LuigiDarkSimeone, @fnavruzov, @RonanFR, @FrancescoFondaco, @QijiaShao, @terbed |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1432 +/- ##
=======================================
Coverage ? 85.59%
=======================================
Files ? 68
Lines ? 6597
Branches ? 0
=======================================
Hits ? 5647
Misses ? 950
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adding a weight argument to TimeSeriesDataSet will cause the trainer to throw an error for NhiTS because of a dimension mismatch in dimension 1.
This is because of unsqueeze operation and it is not solely because of missing values (the error will happen even when there is no missing values).
To fix this we can remove the unsqueeze(-1) and multiply the losses and weight directly.
(Ignore the first few commits as they are for NHitS encode-decoder length which is already merged to main)
This is the commit: (**Remove Unsqueeze operation to solve the mismatch operation)
I confirm this by running the same NhiTS model with weight operation set:
Issues:
#1431
#1040