Skip to content

Fix silent data corruption in the writer, and the packaging and docs - #4

Open
rafa-guedes wants to merge 10 commits into
fix/nodal-correctionsfrom
fix/io-and-packaging
Open

rafa-guedes wants to merge 10 commits into
fix/nodal-correctionsfrom
fix/io-and-packaging

Conversation

@rafa-guedes

Copy link
Copy Markdown
Contributor

Stacked on #3. The base is fix/nodal-corrections, so review only shows
this branch's commits; merge #3 first and GitHub will retarget this to master.

Everything here is IO, packaging or documentation. No prediction maths.

Silent corruption

8548324 -- the oceantide format packs into int16 against a fixed scale
(±20 for amplitudes, 0 to 12000 m for depth) and numpy wraps rather than
saturating, so anything past either end round-tripped as a plausible number of
the wrong sign or magnitude, with nothing logged:

25 m amplitude   ->  -15.0 m
15000 m depth    ->  2999.9 m

Both formats. to_oceantide now raises, naming each offending variable and the
range the format can hold. The upper bound is one quantum below the nominal
maximum because the top quantum encodes to the same integer as the fill value
and would decode back as nan. check_range=False opts out of the extra pass
over the data.

This is a deliberate break. Anything writing out-of-range data was already
producing wrong files and will now fail instead.

6576441 -- and the check immediately earned itself. otis_to_oceantide
divides transport by the depth at U and V nodes; that depth is zero at nodes
bordering land, so where the transport was not also zero the result was ±inf.
The land mask keys off the Z-node depth, so those infinities survived into
the result -- five of them in the netcdf fixture alone -- and into written
files. This is what the divide by zero encountered in divide warnings in the
test output have been reporting all along. No flow passes through a dry node,
so the velocity there is zero. Every previously finite value is bit identical,
checked point by point against the old expression on both fixtures, and h, u
and v now share one mask instead of three slightly different ones.

c663b8f -- predictions inherited the codec encoding of the file the
constituents were read from, so predicting from a zarr store and writing the
result back failed with Expected a BytesBytesCodec. The inherited encoding
also names int16 and the fixed scale the constituents are packed against, and
nothing derived from a packed file should be silently re-packed to match it.

Other fixes

d40b9ef -- 0.7.0 taught the accessor to handle datasets carrying only some of
h, u and v, but to_oceantide still indexed self._obj[["dep"]]
unconditionally and raised KeyError: 'dep' on exactly those datasets.
read_oceantide had the mirror problem.

8835055 -- oceantide on the command line printed "Replace this message by
putting your code into oceantide.cli.main"
. It has been the published entry
point since 0.1.0. Now a small group over the readers and writers that already
exist:

oceantide convert -r otis_netcdf DATA/Model_tpxo9 tpxo9.zarr
oceantide info tpxo9.zarr

8c13d5a -- removes oceantide/ellipse.py, 641 lines of vendored tidal
ellipse code that nothing imports, that has not been runnable since matplotlib
3.0 (plt.hold, plus an exec that assigns to a name read out of a scope it
was never bound in), and that carries its own restrictive licence inside an MIT
package.

60e9d1e -- three packaging problems that all pointed the same way, nothing
was checking:

  • pyyaml is imported on every set_attributes call but was never declared;
    it has always been present because dask pulls it in.
  • requires-python said >=3.8 and advertised 3.8 to 3.11. The current xarray
    needs 3.11, zarr 3 and numpy need 3.12. Floor is now 3.11, and CI checks it.
  • Tests ran only on release. They now run on every push and PR across 3.11 to
    3.13, plus a job pinning zarr<3 -- not padding, the zarr writer branches on
    the major version and picks a different codec import for each, and only the
    installed one was ever exercised.
  • The release workflow ran pip install . 'oceantide[extra,test]', which
    installs the working tree and then installs the released version over the
    top, so the tests gating each release ran against the previous release.

d5a2741 -- the packages exclude listed "docs" and "tests", matching those
names exactly and nothing beneath them. Nothing was nested until this branch
added tests/reference, at which point generate.py started shipping in the
wheel.

Documentation

35801ea -- the README was six words under a title and usage.rst said, in
full, "To use oceantide in a project: import oceantide". Both had been the
cookiecutter defaults since 0.1.0, which is plausibly the single biggest reason
nobody outside Oceanum has picked this up.

The README now covers what it does, quick start, formats, constituents and
phase conventions, and a Scope section saying plainly what it does not do and
pointing at pyTMD, eo-tides and UTide instead. usage.rst covers reading,
predicting, sites, staying lazy, amplitude and phase, writing and the CLI.

Every snippet in both was executed against the fixtures before committing --
one of them failed, which is how c663b8f was found.

The docs also could not build at all as configured: conf.py imported
sphinx_rtd_theme while the docs extra installs pydata-sphinx-theme, and
pulled in sphinx_gallery, IPython and matplotlib extensions the extra does not
install either. The gallery those served has been an empty placeholder for six
years and is gone, along with the build artifacts committed under
auto_gallery. sphinx -b html now succeeds.

Verification

113 tests pass, up from 63. twine check passes on both artifacts, the wheel
carries no test or doc files, and a clean install of the built wheel imports,
predicts and runs the CLI outside the source tree.

__version__ is left at 0.8.0; HISTORY.rst has a 0.9.0 (unreleased)
section and the bump is a release decision.

https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w

The format packs into int16 against a fixed scale and offset: +-20 for
amplitudes, 0 to 12000 m for depth. numpy wraps rather than saturating on
overflow, so anything past either end round-tripped as a plausible number of
the wrong sign or magnitude, with nothing logged. A 25 m amplitude came back as
-15.0 m, a 15000 m depth as 2999.9 m. Both formats, netcdf and zarr.

to_oceantide now checks the range before writing and raises, naming each
offending variable, the span it holds and the span the format can take. The
upper bound is one quantum below the nominal maximum because the top quantum
encodes to the same integer as the fill value and would decode back as nan.
Variables that are entirely missing have nothing to pack and are skipped.

The check costs one pass over the data, which roughly doubles the cost of
writing a lazy dataset, so check_range=False is there for pipelines where the
range is already known. It defaults to True: silently corrupting a file is
worse than reading it twice.

This is a deliberate break. Anything that was writing out-of-range data was
already producing wrong files and will now fail instead.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
0.7.0 made the accessor work on datasets carrying only some of h, u and v, but
to_oceantide still indexed self._obj[["dep"]] unconditionally and raised
KeyError: 'dep' on anything without it -- including every dataset the accessor
had just been taught to handle. read_oceantide had the mirror problem, asking
for h_real, u_real and v_real whether or not the file held them.

Both now take what is there. The zarr writer picked the depth encoding by
skipping dep in the loop and setting it beforehand, which also assumed dep
existed; it now selects the encoding by name inside the loop.

Round trips are tested for h alone, u and v together, h with dep, and the full
set, over both netcdf and zarr.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
otis_to_oceantide divides transport by the depth at the U and V nodes to get
velocity. That depth is zero at nodes bordering land, and where the transport
there was not also zero the result was +-inf. The land mask applied afterwards
keys off the Z-node depth, not the U or V node's, so a Z-node in water next to
one of those nodes kept the infinity: five of them in the netcdf test fixture
alone. They then went into the written file, where the int16 packing turned
them into arbitrary numbers.

This is what the "divide by zero encountered in divide" warnings in the test
output have been reporting all along, and it is what the new packing range
check surfaced when converting OTIS data.

No flow passes through a dry node, so the velocity there is zero. Every
previously finite value is bit identical -- checked point by point against the
old expression on both fixtures -- and h, u and v now share a single mask
rather than three slightly different ones.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
`oceantide` on the command line printed "Replace this message by putting your
code into oceantide.cli.main" and a link to the click documentation. That has
been the published entry point since 0.1.0.

It is now a small group over the readers and writers that already exist:

    oceantide convert -r otis_netcdf DATA/Model_tpxo9 tpxo9.zarr
    oceantide info tpxo9.zarr
    oceantide --version

convert reads any of the four supported input formats and writes the oceantide
format, taking the output format from the extension; --no-check-range passes
through to the writer for pipelines that do not want the extra pass. info
prints the constituents, variables and extent, which is the question usually
being asked of one of these files.

Nothing beyond click, which was already a dependency and previously unused.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
641 lines of Zhigang Xu's tidal_ellipse MATLAB tools, translated to Python by
Pierre Cazenave and vendored from PyFVCOM. Nothing in the package imports it,
nothing exports it, and no test touches it.

It has not been runnable for years in any case: it imports matplotlib at module
scope, which is not a declared dependency, calls plt.hold, removed in matplotlib
3.0, and builds an expression with exec that assigns to a name the enclosing
function then reads out of a scope it was never bound in. Only docs/oceantide.rst
referred to it, by automodule, which would have failed a docs build on any
machine without matplotlib.

It also carries its own licence terms -- the author retains copyright, and asks
that the program name not be changed -- inside a package distributed under MIT.
Carrying that for dead code is not worth it.

Nothing is lost that was reachable. If tidal current ellipses are wanted later
they are a short vectorised operation on the complex u and v the accessor
already holds, rather than a translation of a translation.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
Three packaging problems that all pointed the same way: nothing was checking.

oceantide.core.utils imports yaml to read attributes.yml on every call to
set_attributes, but pyyaml was not a declared dependency. It has always been
installed anyway because dask pulls it in, which is how an undeclared
dependency stays invisible until the day the transitive one drops it.

requires-python said >=3.8 and the classifiers advertised 3.8 to 3.11, which
has not been true for a while: the current xarray needs 3.11, zarr 3 and numpy
need 3.12. The floor is now 3.11, which is the oldest the dependency stack will
resolve for, and CI checks it rather than the metadata asserting it.

Tests ran only on release, so a change could sit on master for months before
anything executed it. They now run on every push and pull request across 3.11,
3.12 and 3.13, plus a job pinning zarr<3. That last one is not padding: the
zarr writer branches on the major version and picks a different codec import
for each, and only the version that happens to be installed was ever exercised.

The release workflow installed `. 'oceantide[extra,test]'`, which installs the
working tree and then installs the released version from PyPI over the top, so
the tests gated on it ran against the previous release rather than the code
being published. It now installs the working tree alone.

.travis.yml has been dead since the move to GitHub Actions.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
Predicting from constituents read out of a zarr store and writing the result
back to zarr failed with "Expected a BytesBytesCodec. Got numcodecs.blosc.Blosc
instead". The lon and lat coordinates carried the encoding of the file they
were read from, including a zarr 2 codec instance, and that followed the
prediction into a store being written in zarr 3.

_write_zarr already stripped these for its own output, which is how the
constant existed to reuse. It now lives in core.utils alongside a small helper,
and predictions are stripped the same way.

Worth doing beyond the immediate failure: the inherited encoding also names a
dtype of int16 and the fixed scale and offset the constituents are packed
against. A prediction is new data on a different scale, and nothing derived
from a packed file should be silently re-packed to match it.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
The README was six words under a title. usage.rst said, in full, "To use
oceantide in a project: import oceantide". Both had been the cookiecutter
defaults since 0.1.0, which is the most likely single reason nobody outside
Oceanum has ever picked this up.

The README now covers what the library does, the quick start, the formats it
reads and writes, the constituents and phase conventions, and a Scope section
saying plainly what it does not do and pointing at pyTMD, eo-tides and UTide
for those. Someone landing on the PyPI page can now tell in thirty seconds
whether this is the tool they want, including when it is not.

usage.rst covers reading, predicting, working at sites, staying lazy,
amplitude and phase, writing, and the command line. Every snippet in both files
was executed against the test fixtures before committing, and the constituent
list is checked against OMEGA.

The docs could not be built at all as configured: conf.py imported
sphinx_rtd_theme while the docs extra installs pydata-sphinx-theme, and pulled
in sphinx_gallery, IPython and matplotlib extensions that the extra does not
install either. The gallery those extensions served has been an empty
placeholder for six years, so it is gone along with the build artifacts that
had been committed under auto_gallery. The extra also listed
sphinxcontrib-programoutput, which nothing used. `sphinx -b html` now succeeds.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w
The packages exclude listed "docs" and "tests", which matches those two names
exactly and not anything beneath them. Nothing was nested until this branch
added tests/reference, at which point generate.py started shipping inside the
wheel as a top level tests.reference package. Widened to "docs*" and "tests*".

Verified by inspecting the built wheel: no test or doc files, 16 package
modules, twine check passes on both artifacts.

Claude-Session: https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant