Skip to content

fix obs dataset names in stream_mixed #431

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix obs dataset names in stream_mixed #431

wants to merge 1 commit into from

Conversation

iluise
Copy link
Collaborator

@iluise iluise commented Jul 2, 2025

Description

fix streams names for NPP ATMS, SEVIRI and SYNOP in steam_mixed to avoid key error not found crash.

Type of Change

  • [ *] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Issue Number

related to this issue: #430

Code Compatibility

  • [* ] I have performed a self-review of my code

Code Performance and Testing

  • [ *] I ran the uv run train and (if necessary) uv run evaluate on a least one GPU node and it works
  • If the new feature introduces modifications at the config level, I have made sure to have notified the other software developers through Mattermost and updated the paths in the $WEATHER_GENERATOR_PRIVATE directory

Dependencies

  • I have ensured that the code is still pip-installable after the changes and runs
  • I have tested that new dependencies themselves are pip-installable.
  • [ *] I have not introduced new dependencies in the inference portion of the pipeline

@iluise iluise self-assigned this Jul 2, 2025
@iluise iluise added bug Something isn't working datasets Anything related to the datasets used in the project labels Jul 2, 2025
@iluise iluise moved this to In Progress in WeatherGen-dev Jul 2, 2025
tjhunter
tjhunter reviewed Jul 2, 2025
View reviewed changes
config/streams/streams_mixed/geos_1.yml
@@ -7,7 +7,7 @@
# granted to it by virtue of its status as an intergovernmental organisation
# nor does it submit to any jurisdiction.

METEOSAT-X, SEVIRI :
SEVIRI :
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change makes logging easier, but I thought these keys follow a specific convention (satellite, instrument).
@clessig , what are your thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should follow the STAC convention for naming, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, that would be best. I will follow the opinion of @clessig on that naming matter though.

config/streams/streams_mixed/npp_atms.yml
@@ -7,7 +7,7 @@
# granted to it by virtue of its status as an intergovernmental organisation
# nor does it submit to any jurisdiction.

NPP, ATMS :
NPPATMS :
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do that, I would suggest either NppAtms (Java) or NPP_ATMS (python)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry, this should be NPP-ATMS according to the STAC convention. I will change it when I get the thumb up from both :)

@iluise iluise linked an issue Jul 2, 2025 that may be closed by this pull request
error "Key not found" in loss computation when training on obs datasets #430
Open
@clessig
Copy link
Collaborator

clessig commented Jul 2, 2025

The problem comes from #283.

@clessig
Copy link
Collaborator

clessig commented Jul 2, 2025

Let's please fixed the cause, not the symptom.

@clessig
Copy link
Collaborator

clessig commented Jul 2, 2025

@kacpnowak : when we re-merge the channel PR we should also fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjhunter tjhunter tjhunter left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@iluise iluise

Labels
bug Something isn't working datasets Anything related to the datasets used in the project
Projects
WeatherGen-dev
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

error "Key not found" in loss computation when training on obs datasets
3 participants
@iluise @clessig @tjhunter