Skip to content
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

Make unstacked_dims property default to ("z",) #2135

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

oliverwm1
Copy link
Contributor

@oliverwm1 oliverwm1 commented Jan 10, 2023

Having to supply unstacked_dims: - "z" to training and validation data configs, but not the test data config for offline diags is confusing for the user. This PR changes the default of BatchesFromMapperConfig.unstacked_dims to ("z",) and modifies the offline diags routine to set this property to None. A warning is printed if this change needs to be made.

Refactored public API:

  • BatchesFromMapperConfig.unstacked_dims has a default of ("z",) instead of None

  • Tests added

Coverage reports (updated automatically):

  • test_unit: 60%

Copy link
Contributor

@mcgibbon mcgibbon left a comment

Choose a reason for hiding this comment

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

There are still some long-standing confusing issues with the way the offline diags re-uses logic from the training for data, since what they need is fundamentally different (offline diags is specific to geospatial data, training is not). But this is practical and an improvement, so I'm in favor.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants