Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Jun 30, 2025

Closes #6530

Note that the "proof" of the fix is to really compare
https://github.com/SciTools/iris/blob/487cc7a96cb0e48256b48f5f9c7124e9f0dc671c/lib/iris/tests/results/integration/netcdf/derived_bounds/TestBoundsFiles/test_save_primary_without_db.cdl
with
https://github.com/SciTools/iris/blob/487cc7a96cb0e48256b48f5f9c7124e9f0dc671c/lib/iris/tests/results/integration/netcdf/derived_bounds/TestBoundsFiles/test_save_primary_with_db.cdl
so .. in the second case, we get this magic extra line


Is this really ready to go ?

The main question there is, probably, more testing may be required - but can we think of any?
One thing to note : the new code is intentionally quite defensive
- i.e. things like "what if the named bounds-var doesn't exist", "what if the terms var doesn't exist"
- but a lot of that is probably not worth testing, since it may be impossible to get the saver to actually produce those cases

@codecov
Copy link

codecov bot commented Jun 30, 2025

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.88%. Comparing base (e4191fb) to head (487cc7a).
⚠️ Report is 141 commits behind head on main.

Files with missing lines Patch % Lines
lib/iris/fileformats/netcdf/saver.py 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6540   +/-   ##
=======================================
  Coverage   89.87%   89.88%           
=======================================
  Files          90       90           
  Lines       23921    23937   +16     
  Branches     4461     4464    +3     
=======================================
+ Hits        21500    21515   +15     
  Misses       1669     1669           
- Partials      752      753    +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.

@pp-mo pp-mo marked this pull request as ready for review July 1, 2025 09:42
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 1, 2025
@bjlittle bjlittle moved this to 🆕 Candidate in 🦋 Iris 3.13.0 Jul 1, 2025
@pp-mo pp-mo moved this from 🆕 Candidate to 👩‍👧‍👦 Parents in 🦋 Iris 3.13.0 Jul 1, 2025
@HGWright HGWright self-requested a review July 4, 2025 09:02
Copy link
Contributor

@HGWright HGWright left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks @pp-mo, and this will work well with #6481, and with this FUTURE flag enabled we won't be generating CF invalid files like hybrid_height.nc anymore.

@github-project-automation github-project-automation bot moved this from 👩‍👧‍👦 Parents to 👀 In Review in 🦋 Iris 3.13.0 Jul 4, 2025
@HGWright HGWright merged commit 3964075 into SciTools:main Jul 4, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In Review to 🏁 Done in 🦋 Iris 3.13.0 Jul 4, 2025
@pp-mo
Copy link
Member Author

pp-mo commented Jul 4, 2025

This looks great. Thanks @pp-mo, and this will work well with #6481, and with this FUTURE flag enabled we won't be generating CF invalid files like hybrid_height.nc anymore.

Thanks @HGWright ! 👍

@pp-mo pp-mo deleted the save_derived_bounds branch July 9, 2025 11:27
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: 🏁 Done

Development

Successfully merging this pull request may close these issues.

Saving derived bounds to netCDF

2 participants