Repository navigation
Fix silent data corruption in the writer, and the packaging and docs - #4
Open
rafa-guedes wants to merge 10 commits into
Open
rafa-guedes wants to merge 10 commits into
rafa-guedes wants to merge 10 commits into
Conversation
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
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.
Stacked on #3. The base is
fix/nodal-corrections, so review only showsthis 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:
Both formats.
to_oceantidenow raises, naming each offending variable and therange 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=Falseopts out of the extra passover 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_oceantidedivides 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 dividewarnings in thetest 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 theconstituents were read from, so predicting from a zarr store and writing the
result back failed with
Expected a BytesBytesCodec. The inherited encodingalso 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 ofh, u and v, but
to_oceantidestill indexedself._obj[["dep"]]unconditionally and raised
KeyError: 'dep'on exactly those datasets.read_oceantidehad the mirror problem.8835055--oceantideon the command line printed "Replace this message byputting 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:
8c13d5a-- removesoceantide/ellipse.py, 641 lines of vendored tidalellipse code that nothing imports, that has not been runnable since matplotlib
3.0 (
plt.hold, plus anexecthat assigns to a name read out of a scope itwas never bound in), and that carries its own restrictive licence inside an MIT
package.
60e9d1e-- three packaging problems that all pointed the same way, nothingwas checking:
pyyamlis imported on everyset_attributescall but was never declared;it has always been present because dask pulls it in.
requires-pythonsaid>=3.8and advertised 3.8 to 3.11. The current xarrayneeds 3.11, zarr 3 and numpy need 3.12. Floor is now 3.11, and CI checks it.
3.13, plus a job pinning
zarr<3-- not padding, the zarr writer branches onthe major version and picks a different codec import for each, and only the
installed one was ever exercised.
pip install . 'oceantide[extra,test]', whichinstalls 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 thosenames exactly and nothing beneath them. Nothing was nested until this branch
added
tests/reference, at which pointgenerate.pystarted shipping in thewheel.
Documentation
35801ea-- the README was six words under a title andusage.rstsaid, infull, "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.rstcovers 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
c663b8fwas found.The docs also could not build at all as configured:
conf.pyimportedsphinx_rtd_themewhile the docs extra installspydata-sphinx-theme, andpulled 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 htmlnow succeeds.Verification
113 tests pass, up from 63.
twine checkpasses on both artifacts, the wheelcarries 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.rsthas a0.9.0 (unreleased)section and the bump is a release decision.
https://claude.ai/code/session_01UwGUDvDZTQWf4MrCig3U1w