Skip to content

Tolerant handling of standard_name and dimension coordinate loading #6338

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

Merged
merged 32 commits into from
Mar 10, 2025

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Feb 26, 2025

🚀 Pull Request

Description

Closes #6318

To-do

  • Raise a warning at the end of loading if anything is present in iris.loading.LOAD_PROBLEMS
  • Docstring and UX improvements to iris.loading.LOAD_PROBLEMS.
  • Get feedback from reviewer on my approach of adding new routines called build_and_add_*
    • Is the philosophy sound?
    • Is the name too clunky?
  • Fix existing unit tests now that I have changed how things work. These failures don't look scary
  • Add new tests for iris.loading.LOAD_PROBLEMS and the new warning

Deviation from the original spec

To avoid making too many enhancements to actions.py - which we consider legacy (#6316) - I decided it was better to create new routines for adding elements to the Cube, and house them in helpers.py, with the rest of the non-Pyke code. If the reviewer is happy with this approach, I will rewrite the spec in the issues.


Consult Iris pull request check list


Add any of the below labels to trigger actions on this PR:

  • benchmark_this Request that this pull request be benchmarked to check if it introduces performance shifts

Copy link
Contributor Author

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Here are some to-do's I would appreciate input on.

@trexfeathers
Copy link
Contributor Author

I forgot to restore the previous state after my doctests are finished.

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 96.29630% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.80%. Comparing base (df5c55f) to head (241a782).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/iris/fileformats/_nc_load_rules/actions.py 78.57% 2 Missing and 1 partial ⚠️
lib/iris/fileformats/_nc_load_rules/helpers.py 96.62% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6338      +/-   ##
==========================================
+ Coverage   89.78%   89.80%   +0.02%     
==========================================
  Files          90       90              
  Lines       23632    23752     +120     
  Branches     4411     4418       +7     
==========================================
+ Hits        21217    21331     +114     
- Misses       1667     1672       +5     
- Partials      748      749       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@trexfeathers
Copy link
Contributor Author

Bit late, but this closes #5171 !

@trexfeathers trexfeathers marked this pull request as ready for review March 7, 2025 13:44
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Posting what I got so far.
Possibly ongoing, though

@pp-mo
Copy link
Member

pp-mo commented Mar 7, 2025

P.S. with a bit more reading, I see that the "new class" idea for LOAD_PROBLEMS causes a lot of problems.
A dictionary of lists has the advantage that the API needs no explaining !
we should discuss

pp-mo
pp-mo previously requested changes Mar 10, 2025
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Pretty solid !
Just a few small changes. mostly to tests

@pp-mo pp-mo dismissed their stale review March 10, 2025 15:28

All requests resolved

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

All good now !

@pp-mo pp-mo merged commit 92d0d63 into SciTools:main Mar 10, 2025
20 checks passed
@trexfeathers
Copy link
Contributor Author

Thanks @pp-mo!

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.

Capture un-loadable NetCDF dimension coords and Cube names
2 participants