Repository navigation
Zarr I/O PR 7: real-world test data and benchmarks - #7349
Merged
Merged
Conversation
Eleven tests over the published subset now in iris-test-data. Four of them pin behaviour no synthetic fixture in this package reaches: the store names its CRS variable in coordinates rather than grid_mapping, so no coordinate system is built; it carries an attribute called dtype, one of the names that used to be shadowed on load; mode='w' replaces an existing store, which was the one of the three modes nothing covered; and a round trip drops the source shard layout, which nothing promises to keep and nothing warns about. Corrects the claim in fixtures.py that the real store replaces the synthetic one. It does not: the synthetic store needs no external data, and it is the more CF-conforming of the two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
Every <U test in this package was on the write side. SciTools#6961 records that these dtypes are not strictly Zarr-supported, and the ESA EOPF Sentinel-2 products carry <U7 and <U3 arrays under conditions/geometry, so the read side needs a defined answer rather than an accident. Covers version 2 and version 3, since the two record dimension names differently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
ZarrSave mirrors NetcdfSave over the same cubesphere cubes, so the two numbers are comparable. Two differences: it passes mode='w', because the Zarr saver defaults to mode='w-' and raises FileExistsError on an existing target while ASV calls a timing function many times between setup() calls; and it has no mesh-only benchmark, because iris.mesh.save_mesh writes netCDF only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
LoadAndRealise gains a Zarr file format beside its NetCDF one, so the two are measured over the same synthetic UM data at the same shapes. create_um_files needed two changes for it. Its reuse check tested is_file(), which is always False for a store directory, so REUSE_DATA would never have held and the data would have been rebuilt on every run. And compression is spelled compressors= rather than zlib=: the Zarr saver rejects netCDF keywords rather than translating them onto codecs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
Six findings, four of them things the published NOAA GFS store does that no synthetic fixture did. The EOPF fixture is dropped on licence grounds and sections 5 and 7 now say so; section 7 keeps the store as cited evidence, because the rules it motivated do not depend on Iris shipping a copy. Also adds the plan this pull request was built from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
They failed instead. _shared_utils.skip_data asks only whether iris-test-data is present at all, and get_data_path does not check that the path it builds exists, so a checkout predating the store gave eleven failures rather than eleven skips. Iris and iris-test-data are versioned separately, so that window reopens whenever one moves before the other. Also moves the xarray import into the one test that needs it, so an install with zarr but without xarray loses that test rather than the whole module. Corrects three things in the design document that this branch's own section 12.4 entry contradicts: section 7 called spatial_ref a grid mapping when the store declares no grid_mapping attribute, said both sources are contributed when only one is, and named the wrong fork. The header table said nothing was blocked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
The published GFS store names its CRS variable in coordinates and declares no grid_mapping attribute, so Iris builds no coordinate system from it. A fallback reading crs_wkt off an unreferenced scalar coordinate was proposed and rejected: section 1 already says a reader is never bent to fit one publisher's files, and the specification is what decides. Section 12.4 now records the decision rather than leaving the question open. Records SciTools/iris-test-data#98 and #99 in section 12.1, and names #98 as what the header table's Blocked on entry waits for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 511ac21a-e935-4d77-baa6-1b8cb42dac17
v2.31 is the first release holding test_data/Zarr/gfs_forecast_sample.zarr, so until now the eleven integration tests over it skipped. Both workflow files pin the version and the comment in ci-tests.yml points at the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
The GFS tests carried both a module skipif on the store's own existence and the standard skip_data decorators. Reviewers objected from opposite sides, and skip_data is the one to keep: it also honours IRIS_TEST_NO_DATA, which the custom guard ignored, so keeping only the custom guard would have run these tests for someone who had asked the suite not to touch external data. The guard existed because the data was unreleased; iris-test-data v2.31 is pinned now, and there is no backwards-compatibility contract with older versions. NetcdfSave and ZarrSave were near-identical, so they become one Save class parametrised over file_format, the way LoadAndRealise and TimeConstraint already are. time_save_mesh is guarded on the format rather than skipped, because ASV's skip facility raises out of setup() and so cannot skip one method for one parameter value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
No technical reason it was absent - the design document named LoadAndRealise and the save benchmarks, so that is what was built. setup_cache already passes the format list straight to create_um_files, so this is one word. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
…in them Section 6 gains the rule the review turned up: a test that writes an array and reads it back moves when the writing library moves, so it answers the reader-against-writer question and not the interoperability one. That needs bytes which do not move, which is SciTools/iris-test-data#100. Section 12.4 records the three other review actions, and 12.1 and 12.5 carry the new companion pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
The unit test for <U dtypes writes an array with zarr-python and reads it back with Iris, so both halves move whenever zarr-python's encoding moves: change it and the test still passes while a file written years earlier stops loading. That is the reader-against-writer question, not the one <U support actually answers. These stores were written once and committed, so they pin the second. One of the tests asserts the on-disk dtype strings, so regenerating the fixtures - which would destroy their only value - fails rather than passing quietly. These tests FAIL until SciTools/iris-test-data#100 merges, releases, and IRIS_TEST_DATA_VERSION is bumped. Committed red deliberately, so that the new file can be reviewed alongside the fixtures it reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 0022a6ec-642a-4844-ae3b-6b8f4421303a
trexfeathers
added
Agentic
Type: Feature Branch
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: c18b0b04-01bf-4ba9-96b0-4d8c8b5cfa02
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: c18b0b04-01bf-4ba9-96b0-4d8c8b5cfa02
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: c18b0b04-01bf-4ba9-96b0-4d8c8b5cfa02
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## brownfield #7349 +/- ##
===========================================
Coverage 91.03% 91.03%
===========================================
Files 106 106
Lines 27148 27148
Branches 4961 4961
===========================================
Hits 24715 24715
Misses 1677 1677
Partials 756 756 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
trexfeathers
added
the
benchmark_this
Contributor
⏱️ Performance Benchmark Report: 4966a51Performance shiftsFull benchmark resultsGenerated by GHA run |
pp-mo
approved these changes
Oct 27, 2026
pp-mo
left a comment
pp-mo
left a comment
Member
There was a problem hiding this comment.
👍 confirmed changes all equivalent to those in https://github.com/trexfeathers/iris/pull/101/changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(posted by Claude Opus)
This is the seventh of nine pull requests delivering native Zarr support for Iris, and it adds the real-world test data and the ASV coverage the programme owes: a published NOAA GFS store loaded and saved by eleven integration tests, committed fixtures pinning fixed-length unicode on the read side, and Zarr added to the save, load and time-constraint benchmarks. Part of #6961. If the integration tests are wrong, Iris ships believing it reads published Zarr when it does not — and what I most want from a reviewer is a second opinion on dropping the EOPF fixture.
No library behaviour changes. Every file here is a test module, a benchmark, a workflow pin or a design document, with one exception:
benchmarks/benchmarks/generate_data/um_files.py, which is benchmark support code and reaches no shipped module.The base. This branch was built on
zarr-io-pr-6and previewed at trexfeathers#101, so its own diff could be read while #7346 was still in review; @pp-mo reviewed and approved it there, and his comments are answered in the commits here. #7346 has since merged, and this branch is now rebased ontobrownfieldwithgit rebase --onto upstream/brownfield zarr-io-pr-6 zarr-io-pr-7, so the diff below is this pull request's own work and nothing else.The fixtures, and the version pin. The GFS store landed in SciTools/iris-test-data#98 and the stored unicode stores in #100, released together as v2.32, which both workflow files now pin — so every test here runs rather than skips. Locally against a v2.32 checkout the Zarr integration package is 86 passed with nothing skipped, and the whole suite is 12876 passed, 61 skipped, none failed. This branch was briefly red on purpose while #100 was still in review, so the new test could be read beside the fixtures it needs; that is over, and the version bump was the whole of the fix.
Important
The published GFS store names its CRS variable in
coordinatesand carries nogrid_mappingattribute, so Iris builds no coordinate system from it andcube.coord("latitude").coord_systemisNone. CF section 5.6 makesgrid_mappingthe way a data variable points at its grid mapping, so Iris is already right and this branch changes nothing. A fallback that readcrs_wktoff an unreferenced scalar coordinate was proposed and rejected by @trexfeathers: a library that second-guesses every file it might meet has given up the specification as the thing that decides. The behaviour is pinned by test so that the absent coordinate system reads as the data's doing rather than as a regression.What is here
The data. A 1.4 MB subset of the dynamical.org NOAA GFS forecast archive, Zarr version 3, contributed to
iris-test-databy SciTools/iris-test-data#98. It keeps the sharding codec, the scalarspatial_refvariable, the two-dimensionalvalid_timecoordinate and the base64_FillValueencoding, because those are what it exists to exercise. That pull request also teaches the index generator to treat a store as one item rather than descending into its chunks.The integration tests.
lib/iris/tests/integration/zarr/test_gfs_sample.py. Loading, the coordinate structure, the attribute handling, a round trip, and xarray opening what Iris wrote. Four of them pin behaviour no synthetic fixture in the package reaches: the grid-mapping finding above; an attribute literally nameddtype, which is one of the names that used to be shadowed on load;mode="w"replacing an existing store, which was the one of the three save modes nothing covered; and a round trip dropping the source shard layout, which nothing promises to keep and nothing warns about.Fixed-length unicode on the read side, twice. Before this branch every
<Utest in the package was on the write side. A unit test now writes a<U3array with zarr-python and loads it back, on both Zarr versions; and an integration module reads two committed stores. The second exists because the first cannot pin what it looks like it pins — write and read move together when zarr-python's encoding moves, so only bytes that were written once and never touched again can show that Iris still reads a file written years ago. A test asserts the encoding strings those stores were written with, so regenerating them — which would destroy their only value — fails rather than passing quietly.The benchmarks. The save benchmarks become one
Saveclass parametrised overfile_format, the shapeLoadAndRealiseandTimeConstraintalready had, rather than a near-duplicateZarrSavesitting besideNetcdfSave. Those two each grow a"Zarr"value beside their"NetCDF"one, andgenerate_datalearns to write a store so they have something to load.Where I would look first
The
_assert_clean_loadfixture intest_gfs_sample.pyescalatesIrisLoadWarningto an error and assertsLOAD_PROBLEMSis empty. That is only correct because the published store happens to load silently; if a future Iris starts recording a problem for it, this module fails rather than quietly covering it, which is the intent, but it does make the module strict.Parametrising the save benchmarks renames the netCDF ones, so
NetcdfSave.time_netcdf_save_cubebecomesSave.time_save_cubeand its ASV history does not carry across. That is a real loss, accepted on review as the lesser one against a near-duplicate class; it is the place I would most expect someone to disagree.time_save_meshis guarded on the format rather than skipped, because ASV's skip facility raises out ofsetup()and so applies to every method in the class.The save benchmarks pass
mode="w". Without it the benchmark raisesFileExistsErroron its second call, because ASV re-runs a timing function many times betweensetup()calls and the Zarr saver defaults tomode="w-".Both new modules skip on
_shared_utils.skip_dataalone. An earlier version also guarded on the store's own existence, becauseskip_dataasks whetheriris-test-datais present and never whether the file is — so on a checkout older than the pinned version these tests fail rather than skip, which is what the branch did on purpose while the fixtures were unreleased. Two reviewers objected from opposite sides and the custom guard went: it did not honourIRIS_TEST_NO_DATA, so keeping it would have run these tests for someone who had asked the suite not to touch external data. Iris andiris-test-dataare versioned separately, so that window reopens whenever one moves first.What I rejected
The ESA EOPF version 2 fixture, which the design document promised. Its licence is a bespoke Copernicus instrument rather than a standard one, and having it read by someone who handles Copernicus terms did not fit the schedule. Before dropping it I checked what it would have covered: the raise when a root group holds no data variables, the version 2 fill-value split, and nested JSON attributes are all already pinned by stores built in
tmp_path. The one gap was reading a fixed-length unicode variable, which is the stored-fixture module above. The store stays in the design document as cited evidence for the rules it motivated; it goes only as a shipped fixture.Replacing the synthetic GFS-like fixture with the real one, which
fixtures.pysaid would happen. The synthetic store needs no external data, so it keepstest_load.pyrunning whereiris-test-datais absent, and it is the more CF-conforming of the two because it gives its data variable thegrid_mappingattribute the published store lacks. The docstring is corrected rather than the fixture deleted.Amending
README.rstiniris-test-data. Its blanket Crown copyright claim has been inaccurate since a CC BY-SA 4.0 CMIP6 file landed there in 2019. That is a SciTools decision, raised as SciTools/iris-test-data#99; this work records provenance per directory instead.No changelog fragment: per the design document, fragments are written once at merge-back.