Conversation
…ible aliases `nc.variables` has been a mapping for a while, but the container itself was not: `nc["t2m"]`, `"t2m" in nc`, `len(nc)` and `list(nc)` all failed, so the first thing an xarray user types did not work. Each new member delegates to `variables`, so there is one enumeration and one refusal message, not two. - add `__getitem__`, `__contains__`, `__iter__`, `__len__`, `get`, `keys`, `values` and `items` on `NetCDF` - add the read-only aliases `data_vars`, `sizes` and `attrs` - add `coords`, built from `get_dimension_values` so the storage-order contract stays in one place - add `dtypes`, `nbytes` and `info(buf=None)` `__getitem__` raises `KeyError` where `get_variable` raises `ValueError`; the mapping protocol needs `KeyError` for `in`, `get` and `dict(nc)` to behave, so the two spellings differ deliberately. `dims` is a **mapping** of name to length, as xarray's `Dataset.dims` is, and is therefore the same object as `dimension_sizes` rather than an alias of `dimension_names`, which is a list. Aliasing a mapping name onto a list would let `nc.dims["time"]` silently return a list index. `nbytes` is computed from each variable's shape and dtype and reads no pixels, so a cube larger than memory can still be sized. It counts data variables only, where xarray's counts its coordinates too.
…rospection
Two modules. The first sweeps seven stores — a plain 4-D CF cube, a container
declaring dimensions its variables do not all use, a packed 2-D store whose
variables report a renamed `subset_y_...` axis, a container holding only
`LabeledArray`s, a grouped store with `group/var` names, a curvilinear store that
reads more names than it enumerates, and a single-variable store — and asserts that
every new member delegates totally rather than enumerating a second time.
Three things are pinned because they look like inconsistencies and will otherwise
be "fixed":
- `nc["nope"]` raises `KeyError` where `nc.get_variable("nope")` raises `ValueError`
- `nc.dims` is a mapping and is therefore not `nc.dimension_names`
- `dtypes`, `nbytes` and `info` all answer with `read_array` patched to raise, so a
`sum(v.read_array().nbytes)` implementation cannot pass
The second asserts `sorted(nc) == sorted(nc.to_xarray().data_vars)` on the 23 stores
where it holds, and gives the three where it does not a named test each: the grouped
store, where an xarray `Dataset` is one flat namespace and holds one size per
dimension name, so 8 names are flattened and 20 variables are skipped — both warned
about; and the GOES and UGRID stores, where the export carries an array CF
classification leaves out of `variable_names`. All three remain reachable through
`get_variable`.
A coverage pass showed every new body exercised, but two kinds of scenario were
missing. Both are now pinned.
A variable subset is a `NetCDF` too, so every new member is reachable on it, and
the answers follow from the members they delegate to being *container* concepts:
`variable_names` is empty, so `len`, `list`, `dict`, `dtypes` and `nbytes` all
report nothing — `nc["temperature"].nbytes` is 0 for a variable that plainly holds
2880 bytes. `dimension_sizes` needs a root group a subset does not have, so `dims`
is `{}` while `dimension_names` still reports four names from the cache built with
the subset. Each is documented on the property it surprises, because the natural
reading of `variable.nbytes` is not what it answers.
That last one settles which member `coords` iterates: `dimension_names`, not
`dims`. Keying off `dims` would throw away the axes a subset can still read, so
`set(coords) <= set(dimension_names)` is the invariant, and the test now asserts
that rather than the `dimension_sizes` version that only held on containers.
Also:
- pin the `"unknown"` arm of `_variable_dtype`, unreachable from any fixture since
no store has a zero-band variable, through a stub
- pin the `LabeledArray` arm of both sizing helpers directly
- assert `values()` and `items()` return the same objects `__getitem__` does, not
merely the same count
- assert a returned mapping or list can be mutated without reaching the container
- assert `info`'s section order and that an attribute-less container still closes
- drop the dead `types.get(name, "unknown")` fallback in `info`; it iterates the
same `variable_names` that built the mapping
…sserting equality Most examples read `nc.data_vars == nc.variable_names` -> `True`, or `nc.sizes == nc.dims == nc.dimension_sizes` -> `True`. That confirms the delegation and teaches nothing: a reader cannot see what any of them returns, and the doctest passes just as well if both sides are wrong together. Every example now prints a real value and then does something with it — the names of a five-variable store and the band counts they reach, the index a pressure level sits at, the megabytes a store would cost to read, the variables with more than one band. `info` prints its whole summary, with the tabs expanded so the example is readable. Also: - give `_variable_dtype` and `_variable_nbytes` examples; both branch on the variable kind and `_variable_dtype` has a documented `"unknown"` return, so neither is self-evident from the signature - add a second example to each member that had only one - add `See Also` to `__contains__`, `__iter__`, `__len__`, `keys`, `values`, `items` and `get`, which had none 48 doctests pass.
|
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.



Description
Tasks T1, T2 and T16 of
planning/xarray/missing-functionality-plan.md— the Tier 1 ergonomicsbatch.
nc.variableshas been a mapping for a while, but the container itself was not:nc["t2m"],"t2m" in nc,len(nc)andlist(nc)all failed, which is the first thing a reader arriving from xarraytypes. Every member added here delegates to
variables, so there is one enumeration and one refusal messagerather than a second list that can drift.
T1 — the mapping protocol on the container.
__getitem__,__contains__,__iter__,__len__,get,keys,valuesanditems.__iter__yields data variables only, matchingvariablesandvariable_names, and the docstring says so — this is thevariables-shaped collision the plan flagged(xarray's
Dataset.variablesincludes coordinates; this class's does not).T2 — the xarray-compatible aliases.
data_vars,sizes,attrs, and a newcoordsbuilt fromget_dimension_valuesso the storage-order contract stays in one place.T16 — cheap introspection.
dtypes,nbytesandinfo(buf=None). None of the three reads a pixel.Three things look like inconsistencies and are pinned by tests so they are not "fixed" later:
nc["nope"]raisesKeyErrorwherenc.get_variable("nope")raisesValueError. The mapping protocolneeds
KeyErrorforin,getanddict(nc)to work at all, andget_variablepredates the mapping andcannot start raising
KeyErrorwithout breaking callers.dimsis a mapping of name to length, as xarray'sDataset.dimsis, and is therefore the same objectas
dimension_sizesrather than an alias ofdimension_names, which is a list. This is the decision theplan asked to be made explicitly; aliasing a mapping name onto a list would let
nc.dims["time"]return alist index silently.
dimension_nameskeeps its name and its list.nbytescounts data variables only, where xarray's counts its coordinates too, and is computed asrows * columns * band_count * itemsize— the band axis is the whole flattened non-spatial stack, so thatproduct is the cube and not a plane. A cube far larger than memory can therefore still be sized.
Five places this diverged from the plan's spec, all recorded in
planning/xarray/missing-functionality-plan.md§3:
sorted(nc) == sorted(nc.to_xarray().data_vars)holds on 23 of the repo's 26 stores and fails on three, so it is asserted as a sweep over the 23 plus one
named test per store where it fails. None of the three is a defect on either side — see How Has This Been
Tested? below.
inforeads each variable's axes from the store, not from the subset.get_variablerenames avariable's y dimension to the window it was cut with (
subset_lat_127_-1_128), which is not a name thefile has, so printing the subset's
dimension_nameswould have put a fabricated axis in the summary._variable_dim_names(rg, name)is used instead. That rename is a pre-existing wart this PR only worksaround; it is worth its own issue.
coordsomits an unindexed dimension rather than mapping it toNone. The plan said "overdimension_names", which would have included the CMIP store'sbndsand every UGRID count dimension. ANonevalue makesnc.coords[name].shapefail for a caller iterating the mapping, and xarray drops themtoo, so a UGRID store's
coordsis{}.dimstook the plan's recommendation. Its "aliasdimension_namesto nothing" is satisfied by leavingthat member alone rather than by removing anything.
dtypesreports the first band's type, not the per-band list. A raster subset reportsdtypeas oneentry per band (all the same, since a band is a slice of one array); returning that list stringified would
have passed a key-set comparison and been useless.
One consequence worth a decision, deliberately not made here.
NetCDFnow has__iter__and__getitem__, sonp.asarray(nc)builds an array of the variable names where it previously gave a 0-dobject array, and
np.asarray(variable)gives an emptyfloat64array.bool(nc)is unaffected —Dataset.__bool__still refuses. Nothing in the package or in the 8524 tests run below depends on either, andnp.asarrayon a container was never a supported operation; but the class already refuses one ambiguouscoercion in
__bool__, and a matching__array__pointing atread_array()would close this one. Left outof this PR as scope creep.
No new dependencies.
Issues
nc["t2m"],"t2m" in ncandlen(nc)all raisedata_vars,dims,sizes,attrs,coords)info(),nbytes,dtypes)Type of change
Check relevant points.
How Has This Been Tested?
Two new modules, 150 tests.
tests/netcdf/structure/test_container_mapping_and_aliases.py— 123 tests, markedcore. Sweeps sevenstores, one per shape the new members have to survive: a plain 4-D CF cube, a container declaring dimensions
its variables do not all use, a packed 2-D store whose variables report a renamed
subset_y_...axis, acontainer holding only
LabeledArrays, a grouped store withgroup/varnames, a curvilinear store that readsmore names than it enumerates, and a single-variable store.
nc[name]andnc.variables[name]agree in type and shape on every name ofevery store;
list(nc) == variable_namesincluding order;dict(nc)round-trips;3 in ncandNone in ncare ordinaryFalserather than aTypeError.nc.get_variable("lat_rho")reads the array,"lat_rho" not in nc, andnc["lat_rho"]refuses with the message that names the accessor which works.KeyError/ValueErrorsplit, anddimsbeing a mapping rather thandimension_names, are eachpinned from both directions.
coords[name]equalsget_dimension_values(name)for every entry;bndsis absent on the CMIP store;a UGRID store's
coordsis{}.dtypes,nbytesandinfoall answer withread_arraymonkeypatched to raise, so asum(v.read_array().nbytes)implementation cannot pass.nbytesis also pinned to one hand-computednumber (
4 * 3 * 5 * 6 * 8) so the sum is not merely self-consistent.infonames every dimension and every variable, writes tosys.stdoutby default, and reports eachvariable's own axes —
float32 area(lat, lon)besidefloat32 ua(time, plev, lat, lon), with nosubset_anywhere in the output.tests/netcdf/parity/test_container_names_match_xarray.py— 27 tests, markedinterop, run by theinterop-testsjob.sorted(nc) == sorted(nc.to_xarray().data_vars)on the 23 stores where it holds, with a test tying thesweep to the fixture directory so the exclusion list cannot grow silently.
to_xarrayflattens 8group/varnames togroup_varbecause aDatasetis oneflat namespace, and skips 20 variables because a
Datasetholds one size per dimension name while eachflight group has its own
recNum. Both are asserted through the warningsto_xarrayraises, notthrough the counts alone — the counts would also match an export that dropped them silently.
DQFand the UGRID store exportsface_node_connectivity, neither of which CFclassification puts in
variable_names. Both are asserted to remain reachable throughget_variable.Regression run, on this branch:
pytest tests/netcdf tests/dataset -m "not plot"— 8524 passed, 82 skipped, 0 failures (11m30s).Run because adding
__iter__and__len__to a core class changes protocol membership;isinstancewas checked against
Mapping,Sequenceand the package's_ArrayLikeProto(all stillFalse; onlyIterableflips), andbool(nc),v + 1,v * 2andv > vwere checked by hand.pytest --doctest-modules src/pyramids/netcdf/netcdf.py— 46 passed, 9 skipped. Every new membercarries executable doctests.
ruff formatandruff checkat the version pinned in.pre-commit-config.yaml(0.15.22) — clean; noline over 120 characters.
Checklist:
plan document records the divergences