-
Notifications
You must be signed in to change notification settings - Fork 90
AIFS-ENS Initial time-step fix #615
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
Conversation
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.
Greptile Overview
Greptile Summary
This PR fixes a critical bug in the AIFS-ENS model where the iterator was incorrectly skipping the initial time-step (0h lead time) and the output was incorrectly including 4 invariant variables.
Key Changes:
- Replaced hardcoded index slicing (
[:92]and[101:]) with dynamic filtering usingself.model.data_indices.data.output.forcingin_fill_inputmethod - Ensures all 13 forcing/invariant variables (4 invariants + 9 time-dependent forcings) are properly removed from output instead of only 9
- Fixed test to validate iterator returns initial condition at 0h lead time by removing
next(p_iter)call - Updated lead time assertion from
6 * (i + 1)to6 * (i)to match corrected iterator behavior
Impact:
- Output now correctly has 100 variables instead of 104 (fixes invariants being incorrectly included)
- Iterator now properly yields initial condition first, matching expected behavior for prognostic models
Confidence Score: 4/5
- This PR is safe to merge with one minor style improvement recommended
- The changes correctly fix a critical bug where invariant variables were incorrectly included in output and the iterator skipped the initial time-step. The logic is sound and internally consistent. One pre-existing type annotation issue was identified (not introduced by this PR) that should be corrected for code clarity.
- No files require special attention - the changes are straightforward bug fixes
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| earth2studio/models/px/aifsens.py | 4/5 | Fixed forcing/invariant filtering logic to use model data indices instead of hardcoded ranges, one pre-existing type annotation issue found but not introduced by this PR |
| test/models/px/test_aifsens.py | 5/5 | Corrected iterator test to validate initial time-step (0h) instead of skipping it |
Additional Comments (1)
|
|
/blossom-ci |
loliverhennigh
left a comment
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.
Nice! Less chance of indexing issues.
|
/blossom-ci |
|
/blossom-ci |
2 similar comments
|
/blossom-ci |
|
/blossom-ci |
Earth2Studio Pull Request
Description
Discussion here: #578
Basically the initial step of the generator was returning the invariants (104 variables) instead of the expected 100.
The iterator test skipped this step so it wasnt caught and got through the previous PR.
Checklist
Dependencies