Skip to content

Zarr I/O PR 2: CFDataset, and the CF variable classes rewritten against it - #7303

Open
trexfeathers wants to merge 38 commits into
SciTools:brownfieldfrom
trexfeathers:zarr-io-pr-2
Open

trexfeathers wants to merge 38 commits into
SciTools:brownfieldfrom
trexfeathers:zarr-io-pr-2

Conversation

@trexfeathers

@trexfeathers trexfeathers commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Part of #6977. Second of seven pull requests into brownfield; see
docs/superpowers/specs/2026-09-21-zarr-io-design.md §5 for the programme
and §12.1 for its state. The implementation plan is in
docs/superpowers/plans/2026-09-24-zarr-io-pr2.md, and ships with it.

What this does

Introduces iris.fileformats.cf.dataset: a pair of narrow abstract classes,
CFDataset and CFDatasetVariable, describing what Iris needs from an array
store. iris.fileformats.netcdf._dataset implements them over netCDF4, and
everything that used to reach through CFVariable into a netCDF4.Variable
now goes through the interface instead — the CF reader, the load rules, the
netCDF loader, and the netCDF saver.

The change that will be most visible to a reader is CFVariable.__getattr__.
It resolved an attribute name against the file and against the backing
netCDF4 object, and cached the answer with setattr. It now resolves against
self.attributes only. A CF attribute called shape, dtype or name used
to be shadowed by the netCDF4 member of that name and silently lost; it
reaches the cube now.

Behaviour changes

Ten, all deliberate. Items 1 to 7 and items 9 and 10 change what Iris
does. Item 8 changes what Iris requires of its inputs, which is still
a behaviour change from the caller's side — a save that used to work now
raises — so it is in the list rather than in a compatibility footnote.

This list has been declared complete three times and grown three times —
two, then eight, then nine, now ten — each time because a review enumerated
something the previous pass had walked past, not because the code changed
underneath it; read "ten" as the number three reviews have reached, not as a
number anyone has proved.

Each item names the test that pins it. Every one of those tests was checked
by reverting the behaviour in the source and confirming the test fails; where
that check found no coverage, the item says so plainly instead of implying
there is a pin.

  1. A CF attribute whose name collides with a member name is no longer
    lost.
    One headline, two unrelated mechanisms, so they are listed
    separately.

    • On save (finding F13). The "don't clobber" guard in
      Saver._set_cf_var_attributes was hasattr(cf_var, name) and is now
      name not in cf_var.attributes. An attribute named shape, size,
      name, dtype, dimensions or mask is written to the file, where the
      guard used to see the netCDF4 member of that name and drop the attribute
      silently.
      Pinned by lib/iris/tests/integration/netcdf/test_attributes.py::TestAttributesNamedLikeNetcdf4Members::test_attributes_named_after_netcdf4_members_are_saved.
      Restoring the hasattr form fails it.
    • On load, for names that are not declared CFVariable properties.
      __getattr__ resolves against self.attributes instead of falling
      through to the netCDF4 object, so the file's value wins and is recorded
      as used rather than being consumed and discarded.
      Pinned by lib/iris/tests/unit/fileformats/cf/test_CFVariable.py::TestShadowedAttributeNames,
      parametrised over SHADOWED_NAMES = ["filename", "cf_name", "spans", "attributes", "cf_data"]. Filtering those names out of the attribute
      mapping fails 15 of the class's 17 tests.
  2. The five declared-property names behave differently again — a different
    mechanism with the same headline.
    dimensions, shape, ndim, dtype
    and size are declared properties on CFVariable and still resolve to the
    storage object: cf_var.shape returns the array shape, not a file
    attribute called shape. What changed is that the read is no longer
    recorded. The old __getattr__ marked such a name as read before
    forwarding to the netCDF4 variable, so _add_unused_attributes dropped it
    and a legitimate user attribute vanished from the loaded cube — even though
    the value returned was never the file's. A declared property short-circuits
    __getattr__ entirely, so nothing is recorded and the attribute survives
    onto the cube. This is emphatically not "the shadowing was removed": the
    member still wins.
    Pinned by lib/iris/tests/unit/fileformats/cf/test_CFVariable.py::TestTypedProperties::test_a_file_attribute_colliding_with_a_typed_property,
    parametrised over all five. Making each property record its own name fails
    all five parametrisations.

  3. CFVariable.attributes is a snapshot taken at construction, so nothing
    on the load path can write to the file it is reading.
    Pinned by lib/iris/tests/unit/fileformats/cf/test_CFVariable.py::TestAttributeAccess::test_attributes_are_a_snapshot_taken_at_construction,
    with lib/iris/tests/unit/fileformats/cf/dataset/test_TrackedAttributes.py::TestMutation
    underneath it for the write-through semantics that make the snapshot
    necessary. Wrapping the backend mapping live instead of copying it fails
    the first; making TrackedAttributes.__setitem__ not write through fails
    the second.

  4. A malformed file that used to raise now loads. Where ncattrs() lists
    a name that getncattr() then refuses — which the netCDF4 library does for
    some malformed files — _NetCDFAttributes substitutes "" and the load
    succeeds. cf/_reader.py's old _getncattr already tolerated it with the
    same default; the leniency now lives one layer down, so the same malformed
    file gets one answer rather than two different ones depending on which
    layer read it.
    Pinned by lib/iris/tests/unit/fileformats/netcdf/dataset/test_NetCDFDatasetVariable.py::TestAttributes::test_unreadable_attribute_becomes_empty_string.
    Removing the except AttributeError fails it.

  5. The exception type on a missing CF attribute moved AttributeError →
    KeyError.
    Call sites that read a CF attribute by attribute access
    (cf_var.long_name) now subscript the mapping
    (cf_var.attributes["long_name"]), and a miss raises KeyError where it
    used to raise AttributeError. Minor, but downstream code catching
    AttributeError around a CF read will see a different exception. There
    are six such sites — four on the load path, and two on the save path,
    which is worth stating separately because a reader of this list would not
    think to look there:

    • Load. cf/_reader.py:493-494 reads standard_name and then
      long_name; with FUTURE.derived_bounds off — the default — both
      subscripts are unguarded, and with it on, standard_name is guarded by
      an in test three lines above while long_name is not.
      _nc_load_rules/helpers.py:1158 reads grid_mapping_name.
      netcdf/ugrid_load.py:380 reads node_coordinates, and :406 reads
      the connectivity's cf_role, inside an assert.
    • Save. netcdf/saver.py:1159 reads standard_name and :1187 reads
      coordinates, both while rewriting a dimensionless vertical
      coordinate's formula_terms.

    A seventh converted read, _nc_load_rules/helpers.py:567, is deliberately
    not in that list: its contextlib.suppress was changed from
    AttributeError to KeyError along with the read, which is exactly the
    handling the six above did not get.

    Pinned by lib/iris/tests/unit/fileformats/cf/test_CFReader.py::Test_translate__formula_terms_derived_bounds::test_raises_when_standard_name_empty_and_long_name_absent
    — for the cf/_reader.py site only, and there for its long_name
    subscript. The other five have no test that fails if reverted. That is
    measured, not assumed, and measured site by site rather than extrapolated:
    softening each subscript to .get() and running the whole of
    lib/iris/tests/unit/fileformats/ and lib/iris/tests/integration/ left
    the suite green every time — same collected total as the unmutated
    control, no new failure at any of the five.

    All five are malformed-input paths — a UGRID mesh variable with no
    node_coordinates or no connectivity attribute, a grid-mapping variable
    with no grid_mapping_name, a formula_terms variable with no
    standard_name, a cube variable with no coordinates — and none is
    reachable from a well-formed file. They ship flagged rather than
    fixed.
    Closing them means writing new tests for five malformed-input
    paths into the final commit of a branch whose whole claim is that it
    changed nothing; disclosing them here is the better trade. A follow-up
    issue covering all six sites — settling once whether a missing CF
    attribute should raise, and which exception — would sit naturally with the
    merge-back changelog; this pull request proposes one rather than opening
    it.

  6. IRIS_RAW differs on the error-recovery path. CFReader now writes
    its synthesised and invalidated bounds links into the attributes mapping
    rather than onto the CFVariable object, and build_raw_cube reads that
    mapping unfiltered. A formula-term or derived-bounds variable that falls
    back to build_raw_cube gains a bounds key it did not have — and where
    the file carried its own bounds, the invalidation writes None over it,
    so IRIS_RAW shows None where it used to show the file's value. Narrow,
    but real.

    The write is pinned; the behaviour change is not. Deleting the
    invalidating write fails
    lib/iris/tests/integration/netcdf/derived_bounds/test_bounds_files.py::test_load_legacy_hh[with_db]
    and lib/iris/tests/unit/fileformats/cf/test_CFReader.py::Test_translate__formula_terms_derived_bounds::test_promotes_non_formula_root_bounds_to_data.
    But replacing it with the pre-PR form — root_var.bounds = None, a plain
    Python attribute that never reaches the mapping and so never reaches
    IRIS_RAW — leaves every test green, including
    test_CFReader.py::TestSynthesisedBoundsLink, whose own docstring is
    explicit that it characterises TrackedAttributes rather than exercising
    _reader.py. So this item joins item 5 as declared-but-unpinned, and for
    the same reason it is flagged rather than fixed here.

  7. A borrowed bare netCDF4.Dataset now decodes character data.
    CFReader and iris.load both accept an already-open dataset. Where that
    object is not already one of Iris's wrappers — what the ncdata /
    Xarray bridge passes — NetCDFDataset.from_existing now wraps it in an
    EncodedDataset, where CFReader previously stored it untouched. Checked
    against a real pre-branch checkout: cf_group["labels"][:] goes
    |S1 (3, 4) → <U4 (3,), and iris.load(bare_dataset) on a file whose
    coordinates names a char(x, strlen) variable goes aux_coords=[] →
    [('labels', '<U4')]. So a char auxiliary coordinate that used to be
    dropped now loads, and a bare borrow behaves like every other input. Two
    qualifications: the wrapping is unconditional and does not consult
    DECODE_TO_STRINGS_ON_READ (only __init__ does), so the read-time
    opt-out cannot be exercised on a borrowed dataset; and NetCDFDataset.mode
    reports the stipulated "r+" for a read-only borrow, which is cosmetic —
    nothing in the library reads it but __repr__.
    Pinned by lib/iris/tests/unit/fileformats/cf/test_CFReader__dataset.py::TestABorrowedBareDataset::test_char_data_is_decoded.
    Removing the wrap fails it.

  8. Saving into an emulated netCDF4 dataset now requires three members it
    did not before.
    This lands on emulators — ncdata and the Xarray bridge
    (Xarray bridge #4994) — not on real netCDF4 datasets. The module docstring of the test
    that covers it says it best:

    Saving through a CFDataset asks an emulating object for three members
    that the saver never used to reach for. All three are public netCDF4 API,
    and all three are now required of an emulator:

    • Dimension.size - the CF layer reads dimension lengths through it.
      __len__ is not an alternative, as _EmulatedDimension explains.
    • Dataset.ncattrs() - read once, as the dataset is wrapped.
    • Variable.ncattrs() - read once per variable, as each is wrapped.

    They are marked below where the emulators define them. This is an
    accepted consequence of the change, not an oversight: there is
    deliberately no fallback for an emulator that lacks them.

    And, at the _EmulatedDimension definition site, on why len() will not
    serve:

    netCDF4.Dimension.size, which is how the length is read back. len() is
    not an alternative: the emulator is put inside a _thread_safe_nc wrapper,
    whose getattr forwards named members but is never consulted for the
    len() protocol.

    The absence of a shim is deliberate: a speculative
    getattr(dim, "size", None) fallback was declined as defensive wrapping
    for an unconfirmed problem, which lib/iris/AGENTS.md bans.
    Pinned by the emulator doubles and the saves through them in
    lib/iris/tests/unit/fileformats/netcdf/saver/test_Saver__user_dataset.py.
    Deleting any one of the three members makes all five emulator saves error
    — confirmed for each of the three separately. This is the only one of the
    ten whose pin is a test double rather than a behavioural assertion,
    which is appropriate: the thing being pinned is an interface demand on
    third-party code, and the double is Iris's only standing description of
    what that code must provide. It is not a live ncdata test.

    A question for a maintainer before this lands: none of this is
    verified against real ncdata. ncdata is not installed in the development
    environment used here, and installing it needs your say-so. Is an ncdata
    round-trip check wanted first? If ncdata turns out to lack any of the
    three members, the fix is one getattr in the dataset layer, not a
    redesign.

  9. Reaching a netCDF4 member through a CFVariable now warns. The old
    __getattr__ answered any name the backing netCDF4 object carried —
    value = getattr(self.cf_data, name), silently. The route survives for
    one deprecation cycle, but it now goes through
    NetCDFDatasetVariable.deprecated_netcdf_member, which issues an
    IrisDeprecation naming the member and pointing at
    cf_var.cf_data.variable. CFVariable is exported in
    iris.fileformats.cf.__all__, so this reaches callers outside Iris:
    third-party code that read cf_var.getncattr or cf_var.group gets a
    warning where it got silence, and under -W error a read that worked
    before now raises. A hasattr() probe that comes back True warns too,
    because the member was fetched; one that comes back False does not,
    because nothing was used. Nothing in Iris itself takes the route — see
    the reach-through run below, which measures that at zero.
    Pinned by lib/iris/tests/unit/fileformats/netcdf/dataset/test_NetCDFDatasetVariable.py::TestDeprecatedNetcdfMember::test_a_reach_through_warns
    and …::TestDeprecatedNetcdfMember::test_the_message_names_the_replacement,
    both pytest.warns(IrisDeprecation), with
    …::TestDeprecatedNetcdfMember::test_a_missing_name_raises_and_does_not_warn
    holding the probe boundary. Deleting the warn_deprecated call fails the
    first two and leaves the third passing.

  10. A dataset the caller hands to iris.save is now mutated.
    NetCDFDataset.from_existing calls set_auto_chartostring(False) on the
    object it borrows, so saving into a caller's already-open dataset turns
    auto-chartostring off on it, permanently, as a side effect of the save.
    The merge base's save path never called it. Two qualifications, both
    narrowing: the load path already did exactly this before this branch
    (cf/_reader.py applied the same call to datasets it borrowed), so the
    change is to the save path alone; and it is inert for anything
    from_existing has to wrap — a bare netCDF4.Dataset, or an emulator —
    because the wrapping EncodedDataset does its own decoding and swallows
    the call rather than forwarding it. What is left is a save into an
    object that is already an Iris thread-safe wrapper, where the call
    reaches the real netCDF4 dataset underneath. Iris's saver reads no
    character data, so nothing here does anything with the setting; it is
    listed because a permanent change to an object the caller still owns is
    caller-visible whether or not it matters.
    Pinned by lib/iris/tests/unit/fileformats/netcdf/dataset/test_NetCDFDataset.py::TestAutoChartostring::test_turned_off_on_a_borrowed_dataset,
    which spies on the class rather than the instance because
    DatasetWrapper.__setattr__ forwards instance-level patching to the
    contained object. Removing the call fails it.

An eleventh that is not a change: spec §5 allowed one behaviour fix, to
CFVariable.spans. Reading the code showed the gap is unreachable —
_NCZARR_SCALAR_DIMENSION only ever appears as a variable's sole dimension —
so this pull request characterises the current behaviour rather than altering
it, and spends the allowance on (1) instead.

Five things a reviewer will want stated

cf_patch still receives netCDF4 objects. The public
iris.site_configuration["cf_patch"] hook has been documented since Iris 1.3
as being handed netCDF4 objects. Both arguments — the dataset and the
variable — are unwrapped before the call, so a CFVariable never reaches it.
An interim version of this branch passed a NetCDFDatasetVariable, which
defines no __setattr__ and so would have swallowed variable.name = value
silently; that was caught and fixed, and is now pinned by
lib/iris/tests/integration/netcdf/test_attributes.py::TestCfPatch::test_cf_patch_receives_a_netcdf4_variable.

.hooks/check_netcdf4_imports.py gained two entries. The repository
forbids import netCDF4 outside _thread_safe_nc;
lib/iris/tests/unit/fileformats/netcdf/dataset/test_NetCDFDataset.py and
lib/iris/tests/unit/fileformats/cf/test_CFReader__dataset.py are added to
its _PERMITTED_SUFFIXES because they must construct a real, bare
netCDF4.Dataset to test the wrapping path that exists for exactly such an
object. This widens a lint allow-list for two test modules; it is not a
behaviour change.

Where a file attribute and an undeclared netCDF4 member share a name, the
file wins now.
That is the load half of behaviour change 1, stated the
other way round, and it is worth saying explicitly because it inverts a
precedence. The stronger statement is that nothing inside Iris depends on
that precedence any more, in either direction: after this branch no reader in
the library reaches an undeclared netCDF4 member through a CFVariable at
all. That is measured rather than asserted — the reach-through run below
promotes this branch's own deprecation warning to an error and gets zero
occurrences. (ugrid_load.py's two error messages did read cf_var.name,
which would have been exactly such a reach; this branch moved them to
cf_var.cf_name, Iris's own name for the variable.) So the inversion is
visible only to a caller outside Iris, reading a colliding name from a
CFVariable — and only for a file perverse enough to carry a CF attribute
named after a netCDF4 member.

Six assertions in
lib/iris/tests/unit/fileformats/netcdf/loader/test__translate_constraints_to_var_callback.py
changed, and that is not a loosening.
They were only ever reachable through
MagicMock auto-vivification — the double returned a MagicMock for any
attribute, so the assertions described the mock, not a variable. Re-measured
against a real backing variable across 36 cases, the old and new forms
diverge in none of them; the new form describes what a real CFVariable
does.

Eight grid-mapping parameter assignments gained coverage they never had —
Mercator's scale_factor_at_projection_origin and the seven in the
PolarStereographic branch, in
lib/iris/tests/integration/netcdf/test_attributes.py::TestGridMappingAttributes.
These are tests added, not behaviour changed: a round-trip against the
pre-branch tree confirmed the values were always reaching the file.

How the negative claim was checked

  • Full suite against the merge base. Baseline (recorded on 2cc0b01d4
    before any of this work): 11724 passed, 66 skipped, 6482 warnings, with
    zero FAILED and zero ERROR lines. This branch: 11992 passed, 66 skipped, 6483 warnings, also zero and zero. The normalised diff of the
    FAILED/ERROR lines is empty on both sides. Read nothing into the
    warning totals.
    Five runs of one unchanging tree gave 6483, 6485, 6484,
    6483, 6483 — the summary names the same 13 warning sites every time, but
    import-time module deprecations are counted once per -n auto worker that
    imports the module, so the total moves with how the tests happened to be
    distributed. An earlier draft of this section explained the 6482 → 6483
    step as one extra iris.save; that was a cause invented for a difference
    inside run-to-run noise, and it is withdrawn. The pass and skip counts are
    stable across those runs and are the ones carrying the claim. The skip
    count is read deliberately and is unchanged — a test that silently stopped
    running is neither a pass nor a failure, so a failure diff cannot see it.
  • +268 passed, accounted for exactly. 200 tests in new modules, plus 67
    added in modified modules (counting parametrisations), plus 6 from two
    inherited mixin methods reaching three modules, minus 5 removed. No
    parametrize decorator was removed and no existing parametrisation was
    widened, so the arithmetic is not hiding a substitution.
  • The boundary. cf/dataset.py imports only abc, collections.abc,
    types, typing and numpy; a token-level scan excluding comments and
    docstrings finds exactly one backend name in executable code,
    deprecated_netcdf_member, which spec §4.2 mandates. saver.py contains
    no import netCDF4 and no _setncattr, and names netCDF4 in exactly 68
    places: three self._dataset.dataset escape hatches (two file_format
    reads and the cf_patch hand-off), two cf_var_grid. and sixty-three
    grid_variable. grid-mapping assignments.
  • The deprecating reach-through fires nowhere in Iris's own paths.
    Running lib/iris/tests/unit/fileformats/ and
    lib/iris/tests/integration/netcdf/ with the reach-through warning
    promoted to an error produces zero occurrences.

Not here

No changelog fragment: feature-branch pull requests defer those to the
merge-back, along with the closing keywords. See §5 of the spec. Behaviour
change 9 — the deprecation on the netCDF4 reach-through — is the kind of
change a fragment would normally accompany, and a follow-up issue for the
six KeyError sites in behaviour change 5 is proposed above; both belong
with the merge-back, where the fragment is written.

The sixty-three grid-mapping parameter assignments in saver.py keep a
netCDF4 handle rather than move to .attributes, because they bypass the
ASCII-to-bytes coercion and moving them would change the CDL — crs_wkt
would stop being NC_STRING. Recorded as Q8 in the spec.

One unrelated defect was found and is not fixed here, and is not a flake.
Running lib/iris/tests/unit/fileformats/netcdf/loader/test_load_cubes.py
before lib/iris/tests/integration/netcdf/test_coord_systems.py in the same
process errors all ten tests in the latter. test_coord_systems.py imports
the unit module as tlc, and its module-scoped autouse fixture only reaches
its yield inside if not hasattr(tlc, "TMP_DIR"), while the unit module
sets that global at import. Unit module first ⇒ the fixture returns without
yielding ⇒ every test in the module errors. It is deterministic on module
ordering, not on scheduling. All three files are byte-identical to the merge
base, so it predates this branch — but it is a latent CI failure under an
unlucky ordering and is worth an issue of its own.

🤖 Generated with Claude Code

trexfeathers and others added 30 commits September 24, 2026 15:22
Thirteen tasks taking the narrow CFDataset/CFDatasetVariable ABC pair from
spec §4.2 and routing every CF attribute and netCDF-API access in cf/,
_nc_load_rules/ and netcdf/ through it.

Two behaviour changes, both about attribute names that collide with a
Python member: such an attribute now reaches the cube on load and the file
on save.  Not the `spans` fix spec §5 anticipated -- that gap turned out to
be unreachable, so Task 7 characterises current behaviour instead.

Fifteen findings from reading and running the code record where the
implementation had to settle something §4.2 left open, and Task 13 writes
those back into the spec so PRs 3-6 are built against the real interface.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 25b4f2ac-c5e0-47dc-9296-aef279fc9573
cf/dataset.py declares what the CF layer needs from a storage format:
typed properties for the storage, and one open-world mapping for the CF
attributes.  TrackedAttributes records which attributes were looked up,
which is how Iris decides what survives onto a loaded cube.

Nothing is wired to it yet.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 56828920-920e-46d8-9a40-9016df2fc836
Implements the CFDatasetVariable interface over a thread-safe netCDF
variable wrapper, plus the netCDF-only members - the VLEN check, the
Xarray-bridge data array, and the backing wrapper itself - that the CF
layer used to reach for through getattr.

Not wired to anything yet.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 56828920-920e-46d8-9a40-9016df2fc836
Opens a netCDF file, or borrows an already-open one, and presents its
variables, dimensions and global attributes through the CF interface.

The NetCDF3 "use nccopy" warning moves here from CFReader, behind an
explicit warn_legacy_format flag: the file format is netCDF vocabulary,
and CFReader is no longer allowed to know it.

Dimension lengths are read via DimensionWrapper.size rather than
len(dimension): the wrapper is a composition object whose __getattr__
forwards ordinary attribute lookups but is never consulted for
implicit dunder-protocol calls, so len() raises TypeError where
.size reaches the same value.

Allowlists the new test's direct netCDF4 import in
check_netcdf4_imports.py, alongside the existing thread-safe-wrapper
and bytecoding-dataset test exceptions: the test deliberately exercises
NetCDFDataset.from_existing() against a bare netCDF4.Dataset, as the
Xarray bridge hands to iris.save / CFReader.

Not wired to anything yet.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 56828920-920e-46d8-9a40-9016df2fc836
Review flagged the hardcoded mode as looking arbitrary. It is
deliberate: netCDF4 exposes no public attribute recording a dataset's
open mode, so a borrowed dataset's true mode is unobservable. "r+" is
correct for this PR's only caller (Saver, a write path) and consistent
with the EncodedDataset wrapping applied just below, which __init__
also applies to every mode but plain "r".

Adds a comment recording that rationale and a test pinning the
stipulated value.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
create_dimension, create_variable and write_handle().  write_handle is
the seam that lets a Dask worker write into a variable after the saver
has closed the file, and is where the Zarr implementation will return a
zarr.Array instead of a reopen-on-write proxy.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
_NetCDFAttributes.__setitem__ applies the ASCII coercion that saver.py's
_setncattr applied, so the coercion stays on the netCDF path - as the
spec requires - instead of following CF attributes into generic code.

_bytes_if_ascii moves from saver.py to _dataset.py; saver.py imports it
back until its own _setncattr goes.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Add a test that materialises .variables before calling create_variable,
so the "if self._variables is not None" branch in create_variable is
covered by something other than a happy-path property rebuild. Verified
by temporarily removing that branch: the new test fails, the rest of the
suite is unaffected.

Also wrap the institution-name line in test_global_non_ascii_round_trips
under 88 characters, without changing the string's non-ASCII content.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
One test body, subclassed per implementation, as the spec's testing
section requires.  PR 4 supplies three fixtures and inherits the lot.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
- Record why the shared contract deliberately does not round-trip
  write_handle() through pickle: the handle carries a backend-injected
  lock whose type depends on the active Dask scheduler, and it is
  unpicklable precisely where nothing ever pickles it (the default
  threaded scheduler), so a naive round-trip would wrongly fail a
  correct backend.
- Accept numpy integers alongside plain int in the chunking assertion,
  since a Zarr chunk shape is plausibly numpy.int64.
- Note that deprecated_netcdf_member is deliberately excluded from the
  shared contract, being an explicitly netCDF4-specific escape hatch.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
CFVariable.__getattr__ now reads self.attributes - a TrackedAttributes -
instead of reaching for arbitrary members of the netCDF4 variable, and no
longer caches what it finds onto the instance.  dimensions, shape, ndim,
dtype and size become declared properties, so structure and file data are
no longer reached for through the same syntax.

The instance cache had to go: it made a re-read after cf_attrs_reset()
invisible to attribute tracking, and tracking is what decides which file
attributes survive onto a loaded cube.

cf_data is still a netCDF4 variable; that swap is later in this PR.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Three classes of input the rewiring could plausibly break: a file
attribute whose name collides with a CFVariable member; getattr with a
default and hasattr, which the loading rules use 86 times between them
and which depend on AttributeError and nothing else; and an attribute
read again after cf_attrs_reset(), which is what removing the instance
cache actually changes.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
A file attribute named after one of the five typed properties no longer
vanishes from the loaded cube. The old __getattr__ recorded such a name as
read before handing off to netCDF4, so _add_unused_attributes dropped it
even though the value returned was never the file's; a declared property
short-circuits __getattr__ and records nothing, so the attribute survives.
That is a deliberate difference, and it deserves a test that says so.

Parametrise the collision test over all five names and assert the tracking
outcome through cf_attrs_unused(), not through attributes[name] - which
would itself record the read the test exists to detect.

Also assert each shadowed member's own value rather than merely that it is
not the file's. The weaker form passed against a member returning None, and
was trivially true of attributes and cf_data, which cannot equal a string.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 99010c7d-5098-4102-9977-77317175fd9b
Committed before the code changes, so the record shows these pass
against unmodified spans() methods.

The design spec calls the missing _NCZARR_SCALAR_DIMENSION check in the
three overriding spans() methods a latent bug.  It is latent in the
strong sense: unreachable.  Each override opens with "if self.dimensions"
and then tests set(source[:-1]), which for the one-element tuple NCZarr
produces is the empty set - a subset of every target.  The overrides
already answer True.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 99010c7d-5098-4102-9977-77317175fd9b
CFVariable.spans and its three overrides each decided for themselves
what a scalar variable is, and only one of the four knew about NCZarr's
_scalar_ pseudo-dimension.  The behaviour is unchanged - see the tests
committed just before this - but the question is now asked in one place.

That matters from PR 4: ZarrDataset derives dimension names from the
store rather than from netCDF's rules, and will ask the same question of
data that has never been through them.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 99010c7d-5098-4102-9977-77317175fd9b
…lar coverage

Review found two gaps in the Task 7 tests, neither in spans() itself:

- test_nczarr_scalar_target_is_spanned_by_a_scalar asserted nothing its
  name didn't already promise elsewhere. Once the source is scalar,
  spans() never inspects the target, so this passed for any target and
  was redundant with test_nczarr_scalar_dimension_spans. Removed.

- _is_scalar() is this task's produced interface - the definition PR 4's
  Zarr backend will read directly - but was only exercised indirectly
  through spans(). Added TestIsScalar with direct cases: no dimensions,
  the NCZarr scalar dimension alone, that dimension alongside another,
  and ordinary dimensions.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 99010c7d-5098-4102-9977-77317175fd9b
94 sites in helpers.py and 3 in actions.py: getattr(cf_var, CF_ATTR_X, None)
becomes cf_var.attributes.get(CF_ATTR_X). The brief's line-based inventory
(66 sites) undercounts because several `getattr(...)` calls wrap across 2-3
source lines and never appear as `getattr(cf_` on a single line; completeness
here is proved by `grep -n 'getattr(\|hasattr('` against the *unmodified*
file, not by the wrapped brief list.

Four sites are not the plain rewrite:

- helpers.py's `_add_or_capture` and actions.py:560 look up a name held in a
  variable. getattr string dispatch is banned by lib/iris/AGENTS.md; a mapping
  lookup is the sanctioned form, and reads as one.
- helpers.py's `get_attr_units` asks whether a flag attribute exists without
  marking it used, so that it still reaches the cube. It said so by reaching
  past the CFVariable to the netCDF4 object; it now says so with .untracked.
- helpers.py's `get_cf_bounds_var` (CF_ATTR_BOUNDS/CF_ATTR_CLIMATOLOGY) is
  deliberately NOT converted and stays as getattr(). iris.fileformats.cf.
  _reader synthesises a `.bounds` link for "newstyle" derived bounds
  (FUTURE.derived_bounds) by assigning `cf_var.bounds = ...` directly onto
  the CFVariable instance, bypassing `.attributes` entirely. Plain attribute
  lookup finds that value before `__getattr__` (and so `.attributes`) is ever
  consulted; `.attributes.get(...)` cannot see it and silently drops the
  derived-bounds link. Converting this site broke
  test_bounds_files.py; reverting just these two lines fixed it. This is a
  genuine gap in the pre-verified "31 CF_ATTR_* constants, zero netCDF4
  API-surface collisions" check: the hazard here is a *different* kind of
  collision (a CFVariable-instance attribute set elsewhere in the codebase),
  not a netCDF4 Variable API member.

No behaviour change for real files: the densest attribute check
(test_netcdf__loadsaveattrs.py, 2059 tests) and the derived-bounds suite are
both fully green, and the wider unit+integration selection reproduces the
baseline exactly (4748 passed, 26 skipped, 10 known F14 errors, 0 failed).

The regression net itself needed repair to prove this. ~20 fake `cf_var`
construction sites across 24 test files modelled the *pre-Task-6* CFVariable:
flat Mock kwargs (`Mock(standard_name=X, ...)`) that plain `getattr` read
directly, never touching `.attributes`. Once the loading rules read
`.attributes`, those doubles could not answer - not a false failure, a
correctly-exposed broken double: it modelled an interface CFVariable no
longer has. Added `CFVariableDouble` (lib/iris/tests/unit/fileformats/
nc_load_rules/helpers/__init__.py), a double with a real `TrackedAttributes`
at `.attributes` and `__getattr__` routing to it, exactly like production
CFVariable - so a test can read `double.name` to build its expected value
and the production `double.attributes.get("name")` path resolves the same
value from the same mapping. Rewired every affected construction site
(and lib/iris/tests/test_netcdf.py::TestNetCDFCRS's hand-rolled `Var` double)
onto it; no test assertion changed. nc_load_rules suite: 332 passed, 0
failed, matching the pre-Task-8 baseline exactly.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
…on, restore types

Four items from review:

- test_get_attr_units.py: add test_flag_values_untracked, the one assertion
  that pins `name in cf_var.attributes.untracked` (not `.attributes`) in
  get_attr_units's flag probe. Confirmed it catches the regression: reverting
  the probe to a tracked read fails this new test.
- helpers.py: comment at the `attr_key="cf_name"` site in build_and_add_names
  explaining why the capture branch there is unreachable today (cf_name is a
  structural member, not a file attribute) and what would happen if
  _build_name_var ever grew logic that could fail.
- CFVariableDouble.__getattr__: add the same recursion guard production
  CFVariable carries (_variables.py:86,275) - dunder names and "attributes"
  itself must never resolve through self.attributes, or a blank instance
  (copy/deepcopy/unpickling via cls.__new__) recurses infinitely instead of
  raising AttributeError.
- test__normalise_bounds_units.py: restore the type annotations removed in
  the previous round (`units: str | None = None, unitless: bool = False) ->
  CFVariableDouble`, `attrs: dict[str, Any]`), and mark the five call sites
  and two structural-attribute assignments with per-line `# type: ignore`
  and a comment, rather than hiding the CFVariableDouble/CFBoundaryVariable
  mismatch by leaving the helper untyped.

No production behaviour changed; nc_load_rules suite is 333 passed (the
existing 332 plus the new guard test), matching baseline plus one.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Seven sites.  The constraint fast path also loses a computed attribute
name and a comment about a CFVariable property cache that no longer
exists.

Four test files build CFVariable/CFDataVariable doubles that no longer
match the interface: test__translate_constraints_to_var_callback.py's
MagicMocks carried standard_name as a Python attribute, which is not
where CFVariable looks any more; test__get_cf_var_data.py and
test__load_cube.py spec a bare MagicMock(spec=CFVariable), which only
allows names CFVariable declares at class level; `.attributes` is
assigned in __init__, not declared, so the spec rejected it until added
explicitly.  `.dimensions` had to be set too, for a different reason:
it IS a declared property, so the spec allows it, but the auto-created
child mock iterates empty and test_cf_data_chunk_control's
per-dimension branch never ran; test_build_and_add_auxiliary_coordinate.py::TestDtype
set scale_factor and add_offset as plain Python attributes on a
CFVariableDouble, which CFVariableDouble's own docstring reserves for
structural members, not CF attributes - the sanctioned route is the
attributes mapping, which _get_actual_dtype now reads.

Fixing the constraint-callback doubles to drive ncattrs/getncattr also
exposed that the "attribute not present -> not a mismatch" branch had
never actually been exercised: the old MagicMock doubles auto-vivify
any attribute access, so `hasattr` was always True and the miss path was
dead code under test. With attributes genuinely absent, that branch now
answers True more often, matching the code's stated intent ("the cube
may still acquire the name later in the load") rather than the previous
coincidental mismatch. Several expected result lists changed to match;
this is a fix to what the doubles were pinning, not a change in
production semantics for real netCDF4-backed variables, where hasattr
and "in cf_var.attributes" already agreed on the miss path before this
commit.

The netCDF storage reaches in _get_cf_var_data - _data_array, datatype,
chunking(), the proxy class - are deliberately untouched: they need
cf_data to be a CFDatasetVariable, which is a later commit in this PR.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Fourteen sites, including the third and last computed attribute name in
the loading path: getattr(mesh_var, connectivity.cf_role).

Five hasattr probes - cf_role, topology_dimension and the three
*_node_connectivity names - become "in mesh_var.attributes", which
still records the attribute as read - otherwise face_node_connectivity
and its neighbours would start appearing as stray attributes on every
mesh cube.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
node_coordinates was the one CF read left as direct attribute access in
an if/elif chain whose other two branches already go through
.attributes.  Mandatory on a mesh topology variable, so it subscripts;
a miss becomes KeyError, as it already does for grid_mapping_name.

CFVariableDouble now seeds ignored=_CF_ATTRS_IGNORE, as production
CFVariable does, so scale_factor and friends start already-read in a
double as they do in a real load.

NameConstraint(long_name=...) was pinned by nothing: every shared test
variable either matches long_name or omits it, and omission is
permissive.  One contradicting variable restores the discrimination.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
CFReader is the one module that writes CF attributes during a load: the
formula-terms code synthesises a bounds link onto a variable that did not
carry one.  Those writes land in the mapping now, alongside the reads
that look for them - moving one without the other would write a link
nothing could see.

Two sites needed a None guard rather than a rewrite.  cf_root_coord and
root_bounds_var can each be None, and getattr(None, "bounds", None)
answers None where None.attributes raises.  The first already had a
comment saying so.

The module docstring now names getncattr, which it always required:
_attributes_source calls it for every name ncattrs() returns, and the
docstring listing "variables", "dimensions" and "ncattrs" without it
predates this PR.

test_CFReader.py's netcdf_variable() mock built CFVariable.attributes
from a hardcoded empty ncattrs(), while CF attribute kwargs (bounds,
coordinates, standard_name, ...) were set as plain Mock attributes -
harmless while _reader.py only read them by direct attribute access,
which falls through to the raw mock on a miss. Routing those reads
through .attributes exposed the gap: it stayed empty regardless of the
kwargs given. ncattrs()/getncattr() are now wired to reflect the same
attribute names, so a mutation made before the CFVariable is built - which
is what every mutating test in this file does, on the line above its
CFReader(...) - is seen by both paths.  CFVariable.__init__ snapshots the
attributes, so a mutation made after it is seen by neither.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
netcdf_variable() derived its ncattrs()/getncattr() from a curated name
list filtered on truthiness.  The list omitted long_name and carried
climatology, and truthiness meant a legitimately falsy attribute could
not be represented at all: standard_name="" produced an empty ncattrs()
where a real file lists the name and returns "".

Both surfaces now come from one presence-keyed dict of the CF kwargs the
helper was given, and long_name is one of them.  That makes the
standard_name-or-long_name fallback in the formula-terms promotion
reachable in a unit test for the first time; two tests now pin it,
including the empty-standard_name-and-no-long_name case that raises.

This also moves the freeze point.  ncattrs()/getncattr() used to resolve
through getattr(ncvar, ...) on every call; they now read cf_attributes,
a dict fixed the moment netcdf_variable() returns.  So no post-call
mutation reaches .attributes any more, whatever its timing relative to
CFReader(...) - narrowing what 8bfbb25's last paragraph described.
Absence is now expressed by not passing the kwarg at all, which is why
test_derived_bounds_promotes_reference_terms's "absent standard_name"
case had to stop deleting the attribute after construction and build
the variable without standard_name from the start.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
The swap this PR was for.  CFReader opened a netCDF dataset and handed
its variables straight out; every CFVariable therefore wrapped a
netCDF4.Variable, and so did every consumer downstream.  It now opens a
NetCDFDataset, and cf_data is a CFDatasetVariable.

Nothing outside iris.fileformats.netcdf calls netCDF4 API any more.  The
twelve identify() reads, CFVariable.__init__, CFReader's mesh probe,
grid-mapping parse and global attributes, and the loader's five storage
questions all go through the interface instead.  _getncattr is gone:
_NetCDFAttributes absorbs the malformed-file case it existed for.

CFVariable copies the attribute mapping rather than wrapping it.  The
storage object's mapping writes through to the file, and CFReader
synthesises a bounds link during load, on a file opened read-only.

ugrid_load.py's mesh-location read no longer goes via cf_var.location,
which now collides with CFDatasetVariable.location (the dataset's own
path) rather than the UGRID node/edge/face location: it reads the
already-validated local instead.

The test doubles that stood in for a netCDF4 variable stand in for a
NetCDFDatasetVariable now.  Doubles that hand cf_data a plain array
gain a small RealArrayCfData wrapper for the is_emulated/
is_variable_length gates _get_cf_var_data now checks unconditionally,
and the ones in test__get_cf_var_data.py gain a spec= so that an
invented member is an error rather than a mock.  Assertions follow the
type change throughout: test_CFReader.py's cf_data identity checks
become cf_data.variable checks, and test_CFVariable.py's
attribute-materialisation test is replaced by a snapshot test of the
same construction-time contract.

Part of SciTools#6977

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
CFVariable.__getattr__ falls back to the backing variable for any name
the CF attributes do not supply.  That was how the class worked, and
third-party code will have relied on it, so it stays for one release
cycle - but it now says so.

The warning waits until here because it could not have been switched on
earlier: consumers came off the fallback one task at a time, and a
warning during that would have fired once per attribute per variable per
load.  ugrid_load.py's own error messages were the one surviving
reach-through - cf_var.name rather than cf_var.cf_name - found by
running the netCDF, CF and integration suites with the warning enabled;
fixed here alongside it, since that is what proves the route unused.

It warns only on success.  hasattr() probes reach this method, and a
probe that comes back False has not used anything.

Part of SciTools#6977

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
…overage gaps

A: NetCDFDataset.from_existing wraps anything lacking THREAD_SAFE_FLAG in an
EncodedDataset, unconditionally. Before this branch, CFReader stored a
borrowed dataset untouched, so a bare netCDF4.Dataset - what the Xarray
bridge hands to iris.load - kept its character data as raw bytes; it is
now decoded. This is a genuine, previously-undocumented behaviour change
on a public entry point, not a defect: a char auxiliary coordinate that
used to be silently dropped now loads. Pinned with
TestABorrowedBareDataset in test_CFReader__dataset.py, which needs a
function-local `import netCDF4` to build a genuinely bare dataset - added
to check_netcdf4_imports.py's allow-list, following the pattern already
used for test_NetCDFDataset.py. Documented the wrap's unconditional
nature (it does not consult DECODE_TO_STRINGS_ON_READ) at the wrap
itself.

C: the "r+" stipulation's comment in NetCDFDataset.from_existing claimed
one caller, a write path - false since 936ef88 added CFReader's
borrowed-read branch as a second, read-only caller. Rewritten to name
both callers and note nothing reads NetCDFDataset.mode but __repr__.

D, E, G: three coverage gaps the review found by mutation, each
restored with a test I applied the mutation to and watched fail before
reverting from a /tmp copy:
- _NetCDFAttributes's "read once, at construction" contract had no test
  since 936ef88 replaced the call-count test with a snapshot test
  instead of adding this one alongside it.
- CFReader's borrowed-path wiring of its own warn= into
  NetCDFDataset.from_existing(warn_legacy_format=...) was untested; the
  owned-path equivalent already was.
- test_getattr_of_a_non_attribute_reaches_the_variable lost its
  __getattr__-does-not-cache guard when it gained an
  assert_called_once_with; restored alongside it.

F: renamed test_only_unread_attributes_reach_the_cube to
test_an_unread_attribute_reaches_the_cube and recorded, in a comment,
that the fixture cannot discriminate the complementary "only" direction
- every attribute it reads is filtered by saver.py's _CF_ATTRS before
tracking is consulted - rather than leave the name claiming more than
the test pins.

Part of SciTools#6977

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
iris.fileformats.netcdf.save() accepts an open dataset in place of a
path, and ncdata uses that to translate between Iris and Xarray by
passing an object that only emulates one.  Nothing in the suite tested
it, and the next commit rewrites every line it runs through.

Written against unmodified code, so it is green on arrival.  That is
what makes it a guard rather than a specification.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Saver now owns a NetCDFDataset.  Dimensions, variables, attributes and
data writes all go through the interface, so the saver no longer knows
what kind of file it is writing.

Three netCDF-only reads remain, all through the explicit
'self._dataset.dataset' escape hatch: the file-format check that decides
whether int64 is available, and the third-party cf_patch hook, which is
documented as receiving a netCDF4 dataset.  Grep for it to find them.

The grid-mapping variable keeps a netCDF4 handle too.  Its sixty-three
parameter assignments bypass the ASCII-to-bytes coercion every other
attribute goes through, so converting them would change the file - a
separate change, recorded as finding F8.

One deliberate behaviour change: a coordinate attribute whose name
collides with a netCDF4 Python member - 'shape', 'size', 'dtype' and the
rest - used to be dropped silently, because the "don't clobber" check
asked hasattr().  It asks the attribute mapping now, and the attribute
reaches the file.

NetCDFDataset needed two changes to carry the saver.  'closed' now asks
the file as well as the flag, because a borrowed dataset is closed by
whoever opened it and the flag is never set; complete() has to refuse on
the file's answer, not on its own.  And create_variable() now registers
in the variables mapping unconditionally, materialising it first if need
be.  Keeping an already-built mapping in step was not enough: each
NetCDFDatasetVariable caches its own attribute values, so a second
wrapper for one variable loses attributes written through the first -
and the saver creates a variable, then looks it up by name to write
more.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
cf_patch is a reserved iris.site_configuration hook, documented since
Iris 1.3 as receiving netCDF4 objects.  Saving through a CFDataset
unwrapped the dataset it is handed but not the variable, so a
third-party hook began receiving a NetCDFDatasetVariable: setncattr()
raised, and a plain 'variable.name = value' went nowhere at all, a CF
variable having no __setattr__.  Both arguments are unwrapped now, and
a test installs a real hook and reads the file back afterwards.  The
existing test installed a Mock and never looked at what it was passed,
so it could not have caught either failure.

Three things this branch changed or rewrote had no test that would
notice them being undone:

- An attribute named after a netCDF4 Python member - 'shape', 'size'
  and the rest - used to be dropped in silence and now reaches the
  file.  Reverting that check to the old hasattr() left the whole suite
  green.
- Eight of the sixty-three grid-mapping parameter writes were read back
  by nothing.  They are correct, and byte-identical to the pre-branch
  tree, but this change rewrote all sixty-three of them by hand, and a
  mistake in one is silent: they are assigned straight onto the netCDF4
  variable rather than through the attribute mapping.
- The grid-mapping unit test said 'assert a, b', which is an assertion
  with a message rather than an equality.  It is an equality now.

Also corrected: a comment saying sixty-one assignments where there are
sixty-three, and five docstrings still describing cf_var_cube as a
netCDF4 variable, where internally it is now a NetCDFDatasetVariable.
The cf_patch unwrap does not apply to those - it is at the call site.

Saving through a CFDataset asks an object that only emulates netCDF4 -
ncdata, the Xarray bridge - for three members the saver never used to
reach for: Dimension.size, Dataset.ncattrs() and Variable.ncattrs().
All three are public netCDF4 API.  They are recorded where the test
emulators define them, deliberately with no fallback for an emulator
that lacks them.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Six things the spec's §4.2 sketch did not settle, which the
implementation had to.  None is a correction: §4.2 called its own listing
"deliberately small: only what the CF variable classes actually need",
and this is what they turned out to need.

PRs 3 to 6 are written against §4.2, so it has to be the real interface.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
trexfeathers and others added 3 commits September 25, 2026 04:09
The plan's last Definition-of-Done bullet asks for exactly this: "any step
the implementation proved wrong corrected in place". Nine corrections, each
marked in the text so a reader can see what moved and why.

The behaviour-change count is the important one. The Definition of Done said
two; the decision ledger records eight, and two of those eight are only
partly pinned by tests. A pull request whose central claim is "nothing else
changed" is only as good as its list of exceptions, so the list is now
counted from the rulings rather than from this document, which had been
wrong about the number twice.

The rest: cf/dataset.py has never been free of the *word* netCDF and never
could be, since spec 4.2 mandates deprecated_netcdf_member - so the boundary
bullet and Step 7's grep now check imports and executable tokens instead of
prose. Step 7's other greps become allow-lists, its count goes 66 -> 68 on
the corrected grid-mapping total, and its -W error::IrisDeprecation check is
replaced: that form exits 4 during conftest import on an unrelated,
pre-existing deprecation, so it never ran. Step 6's diff regains the
normalisation 6.3 prescribes. The grid-mapping count goes 61 -> 63 in all
nine places. Task 12's summary said two self._dataset.dataset escape hatches
where its own step text says three. Steps 3 and 4's spec entries go from five
decisions to thirteen. And Step 8's body is replaced with the one that was
actually written.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Review of Task 13 raised five Important and four Minor findings; these are the
accepted ones. Documentation only - no library behaviour changed.

The important one is behaviour change 5. The body said the AttributeError ->
KeyError conversion happened at three call sites; there are six, two of them
on the *save* path in saver.py, where a reader of a list headed "behaviour
changes" would not think to look. The three new ones were measured rather than
extrapolated from the other three: softening each subscript to .get() and
running unit/fileformats/ and integration/ leaves the suite identical to an
unmutated control, so five of the six have no test that fails if reverted. The
gaps ship disclosed rather than closed, and the body states that as a decision
instead of leaving it open as "Reviewer's call" - closing them would mean five
new tests for malformed-input paths, none reachable from a well-formed file,
in the final commit of a branch whose whole claim is that it changed nothing.

A ninth behaviour change joins the list: reaching a netCDF4 member through a
CFVariable now emits an IrisDeprecation where it used to be silent, and that
reaches callers outside Iris because CFVariable is exported. It appeared only
under "Not here", so the defect was placement rather than concealment. Its pin
was verified by reversion like every other item's - deleting the
warn_deprecated call fails two of the three TestDeprecatedNetcdfMember tests
and leaves the third passing. Every count word in the body was then re-derived
by command rather than adjusted by eye, after this branch shipped a wrong
number twice; that turned up "seven PolarStereographic attributes", which are
seven assignments over six attribute names.

The rest: the comment at helpers.py:1278 described a `cf_var.bounds = ...`
assignment that does not exist on this branch and an attributes mapping that
cannot see it, when in fact the link is stored in that mapping like any other
attribute - and a stored None there means "invalidated", which this call site
cannot tell from its own default. The load-half paragraph cited
ugrid_load.py's `cf_var.name` reads, which this branch moved to `cf_name`; the
true statement is stronger, and is what the reach-through-as-error run
measures at zero. The plan described F14 twice as xdist scheduling, where it
is deterministic on module import order. Task 11 Step 16 kept the
`-W error::IrisDeprecation` command that exits 4 at collection and so never
ran; Step 7's copy was corrected and this one was missed. And spec 4.2 gains
__enter__ / __exit__, which the implementation has and the Definition of Done
requires that block to match.

pr-body.md and the plan's embedded copy of it are byte-identical, verified by
extracting the block from the plan and diffing it. Note also that 11ba437's
message says "Nine corrections" where there were ten - left alone, because
history is not rewritten for a message count.

Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
Part A, the one that matters: saving a factory with an absent dependency
raised AttributeError on this branch where the merge base succeeded. The
rewrite of boundsterm_varname replaced getattr(termvar, "bounds", None),
which tolerated termvar being None, with termvar.attributes.get("bounds"),
which does not - and termvar is None whenever _name_coord_map has no name
for the coordinate, as it has none for an absent orography. Restores the
tolerance and pins it with a save/read-back test asserting both
formula_terms strings, since a fix that silently dropped the orog term
would also have stopped raising.

The rest are gaps rather than faults:

- The unlimited-dimension contract member is now exercised. It accepts
  either sanctioned answer, create or NotImplementedError, and rejects the
  third: quietly creating a fixed-length dimension instead.
- _GETATTR_RECURSION_GUARD's comment justified its narrowness by a probe
  that no longer reaches __getattr__. Replaced with the reason that holds:
  a broader exclusion would hide real CF attributes, _Encoding and
  _FillValue among them.
- CFVariable.filename's docstring sat before the try block, attached to
  nothing.
- emulated_data_array's setter refused nothing, while its getter refused.
  Unguarded it would have written a file attribute named _data_array,
  because VariableWrapper.__setattr__ forwards every set to the contained
  object.
- NetCDFDataset.closed's fallback for a borrowed emulator with no isopen()
  had no test; ncdata's emulator has one, so nothing else reached it.
- Four added lines exceeded the 88-character limit.

The pull request body gains a tenth behaviour change, not a tenth change:
from_existing turns auto-chartostring off on a dataset the caller still
owns, which the merge base's save path never did. The load path already
did exactly this, and the call is inert for anything from_existing has to
wrap, but a permanent change to a caller's object belongs in the list. The
suite figures are re-measured to match: 11989 passed, +265.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
@trexfeathers trexfeathers added Type: Feature Branch Highlight this for a feature branch Agentic labels Sep 25, 2026
Part of SciTools#6977

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.11321% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.79%. Comparing base (6d61c5d) to head (a2bca4c).

Files with missing lines Patch % Lines
lib/iris/fileformats/netcdf/_dataset.py 96.77% 7 Missing ⚠️
lib/iris/fileformats/netcdf/ugrid_load.py 81.25% 0 Missing and 3 partials ⚠️
lib/iris/fileformats/_nc_load_rules/helpers.py 98.91% 1 Missing ⚠️
lib/iris/fileformats/cf/dataset.py 99.20% 1 Missing ⚠️
lib/iris/fileformats/netcdf/saver.py 99.21% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           brownfield    #7303      +/-   ##
==============================================
+ Coverage       90.61%   90.79%   +0.17%     
==============================================
  Files              96       98       +2     
  Lines           25859    26204     +345     
  Branches         4802     4819      +17     
==============================================
+ Hits            23433    23791     +358     
+ Misses           1668     1657      -11     
+ Partials          758      756       -2     

☔ View full report in Codecov by Harness.
📢 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 trexfeathers left a comment •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Two regressions were reproduced against the merge base: silent corruption of deferred string writes to borrowed datasets, and failure to load NetCDF files with Dask's process scheduler. Details are attached inline.

Created by Open AI Codex 🤖

Comment thread lib/iris/fileformats/netcdf/_dataset.py Outdated
Comment thread lib/iris/fileformats/netcdf/_dataset.py
trexfeathers and others added 2 commits September 25, 2026 09:01
Both are behaviour changes this branch introduced without documenting
them, which is exactly what the pull request's central negative claim
forbids. Neither adds an item to the list of ten: they are regressions
fixed before merge, not behaviour the branch means to introduce.

A write handle now always encodes. NetCDFDatasetVariable.write_handle
chose its proxy class from the variable's wrapper type, on the stated
premise that an unencoded variable is unreachable from the saver. It is
not: NetCDFDataset.from_existing only wraps a dataset that lacks
THREAD_SAFE_FLAG, so a borrowed _thread_safe_nc.DatasetWrapper keeps
plain variables, and a deferred save into one took the unencoded branch.
Lazy U3 data saved that way reached the file corrupted - ['abc', 'def']
read back as ['aaa', 'ddd']. Restore the merge base's rule that writes
are always encoded, since write-side string encoding is not selectable.

A read no longer makes a write lock. NetCDFDataset handed every variable
the dataset's lock as it built them, and that lock comes from
_dask_locks.get_worker_lock, which refuses the process scheduler because
the *saver* cannot use it. Merely listing variables is a read, so
CFReader._has_meshes made iris.load() fail under
dask.config.set(scheduler="processes"), reporting a scheduler "not
supported by the Iris netcdf saver" in the middle of a load. Variables
now take a write_lock_factory and call it inside write_handle(), so the
lock is still one per dataset - which is the point of the dataset owning
it - but is made only when a write actually needs it.

The unit test that pinned the unencoded-proxy expectation is inverted
rather than deleted, and its sibling renamed so the pair reads as the two
halves of one rule. The two tests that observed the lock at construction
time now observe it at write time, and additionally pin that no lock
exists before a handle is asked for. Two regression tests are added: a
borrowed-dataset string round trip asserting on the file's contents, and
a load under the real process scheduler.

Re-derived the suite numbers in the PR body by collection, per module:
11989 -> 11992 passed, and +265 -> +268 with 198 -> 200 tests in new
modules and 66 -> 67 in modified ones. Skips and warnings unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
The pull request body explained the 6482 -> 6483 step in the full suite's
warning total as the legacy attribute-handling deprecation raised once
more by an added save. Measurement does not support that. Five runs of
one unchanging tree give 6483, 6485, 6484, 6483, 6483: the summary names
the same 13 warning sites every time, and the total moves only because
import-time module deprecations are counted once per -n auto worker that
imports the module, so it tracks how the tests were distributed.

The body's whole argument is that its numbers were measured rather than
narrated, so an explanation fitted to noise is the one kind of error it
cannot afford. Replace it with the measurement, say plainly that the
warning totals carry no weight, and name the pass and skip counts - which
are stable across those runs - as the ones the claim rests on.

Prose only. No suite number changes, and the list of documented behaviour
changes stays at ten.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: de091bc1-c2db-4617-a9da-698262f2ccf4
@trexfeathers

Copy link
Copy Markdown
Contributor Author

Both findings fixed in 0c5ac2e30; ed0d0f766 corrects the body.

Neither becomes an eleventh documented behaviour change — they're regressions fixed before merge, not behaviour this branch means to introduce, the same call made earlier for the boundsterm_varname regression. The list stays at ten.

Two things these findings taught me, both now reflected in the body:

  • P1 sits on exactly the path documented behaviour change 10 describes. Disclosing a side effect is not the same as auditing the path it runs on.
  • Re-deriving the suite numbers turned up something else: 6483 warnings is not reproducible. Five runs of an unchanging tree gave 6483, 6485, 6484, 6483, 6483 — import-time deprecations are counted once per xdist worker. The body had explained the baseline's 6482 → 6483 step as one extra iris.save; that was a cause invented for noise, and it's withdrawn. Pass and skip counts are stable and are what the claim rests on.

Suite now 11992 passed, 66 skipped (was 11989/66) — the +3 are the three added tests.

🤖 Generated with Claude Code

@trexfeathers

Copy link
Copy Markdown
Contributor Author

Follow-up review by Codex, verified at commit ed0d0f766306749ff20a8ab07ecd735125752bb1.

Both original concerns are satisfactorily resolved:

  • P1 — borrowed dataset string writes: Write handles now always use EncodedNetCDFWriteProxy, preserving Unicode strings when saving lazy data into a borrowed, unencoded DatasetWrapper.
  • P2 — process-scheduler loading: Reading variables no longer creates saver locks. Variables use a factory to obtain the shared dataset lock only when a write handle is requested.

Both regression tests reproduce the original failures on the pre-fix commit and pass at the verified commit. All 118 dataset tests and 31 scheduler/deferred-save integration tests also pass, with no skips.

A broader run reported 401 passing tests before a segmentation fault during interpreter shutdown. The corresponding pre-fix run also crashed during shutdown after reporting 398 passes, so that crash predates these fixes. The broader run should therefore not be treated as a clean test-process exit.

No remaining concerns with these two fixes.

trexfeathers and others added 2 commits September 25, 2026 13:43
Every module apidoc turns into an API page must carry exactly one
sphinx-needs item, written in its module docstring. This module was added
without one, so the page generated from it had none and the documentation
build reported an error.

Takes the form used by its sibling, the `iris.fileformats.cf` package.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in the v3.16.1 mergeback from main. Seven of its eight commits merge
cleanly; SciTools#7293 ("Rebase SciTools#7281 onto v3.16.x") conflicts in four files, because
it adds byte<->string encoding to exactly the emulated-variable branches of
_get_cf_var_data and _lazy_stream_data that this branch rewrites.

Resolution:

- SciTools#7293 reaches for the raw netCDF variable two different ways - cf_var.cf_data
  on load, cf_var._contained_instance on save - because
  VariableEncoder.from_var rejects an EncodedVariable outright: the wrapper
  presents char data as strings one dimension shorter, so it cannot describe
  the encoding it is hiding. NetCDFDatasetVariable.unencoded_variable now
  strips that wrapper, and returns anything else unchanged. One accessor
  serves both call sites and is correct whether or not the variable is
  wrapped.
- loader.py and saver.py keep this branch's emulated_data_array accessors and
  re-apply SciTools#7293's encoding through unencoded_variable.
- Both new parametrised tests from SciTools#7293 are rewritten onto this branch's
  spec'd NetCDFDatasetVariable mocks.
- Dropped two imports SciTools#7293 adds to loader.py but never uses, CFDataVariable
  and EncodedVariable. They are dead on upstream/main too.

Caveat: ncdata is not installed in iris-dev, so nothing in the suite exercises
the emulated path end to end, on either side of this merge. The new tests are
mock-based and would not catch unencoded_variable being the wrong choice; that
rests on from_var's documented contract and EncodedVariable being a
VariableWrapper subclass.

netcdf unit + integration: 726 passed, 22 skipped, matching the pre-merge
control run of 720 passed, 22 skipped (+6 = the new tests from SciTools#7293). The
test_coord_systems.py errors are the ordering artefact documented in
lib/iris/tests/AGENTS.md and appear in both runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 09902815-5f62-42a6-b0ef-4c4903ad4a3b

This branch has not been deployed

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

Labels

Agentic Type: Feature Branch Highlight this for a feature branch

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant