Fix silent truncation crash for short forecast data#254
Open
Ayushhgit wants to merge 2 commits intomllam:mainfrom
Open
Fix silent truncation crash for short forecast data#254Ayushhgit wants to merge 2 commits intomllam:mainfrom
Ayushhgit wants to merge 2 commits intomllam:mainfrom
Conversation
Member
|
Good catch @Ayushhgit! Could you add a test to confirm your implementation works please? thank you :) |
Author
|
@leifdenby , I have added a test function to confirm that my implementation works. |
Collaborator
|
@Ayushhgit I must admit, I did not remember this PR and already reviewed this more recent one here: #312 It solves the same issue. If you don't mind, would you head over there and discuss the implementation. Especially the tests your implemented are a bit different. |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Summary of the changes:
n_forecast_stepsbounds validation check insideWeatherDataset.__len__to assert that the available forecast steps length can satisfy both the required autoregressive steps (ar_steps) and the required forcing steps (num_past_forcing_steps+num_future_forcing_steps).Motivation and context:
Currently, len validates available data length against only the AR steps, completely omitting the extra forcing padding limits. If your datastore's forecast duration is slightly shorter than the maximum forcing offset boundaries,
xarray.iselsilently truncates the dimension slices instead of failing. When PyTorch Lightning attempts to collate the slices, the batch tensors have randomly mismatched sequence dimensions leading to incredibly cryptic runtime shape mismatch crashes. This PR safely prevents the silent truncation at initialization.Dependencies:
None
Issue Link
N/A
Type of change
Checklist before requesting a review
pullwith--rebaseoption if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee