Regular Coordinates - #78
Merged
Merged
Conversation
- Privatize is_valid_sampling_interval, assign_sampling_interval, isvalid to _-prefixed versions - Add @OverRide decorators and implement _slice, _concat, _to_dataset, _collect_from_dataset, from_block - Remove debug print statements and dead/broken methods (decimate, simplify, from_array, to_dict) - Fix _concat to take the max tolerance when appending coordinates
Make the previously-unusable FixedInterpCoordinate a working RegularInterpCoordinate (ctype "reginterp"): a piecewise-linear coordinate carrying an enforced nominal sampling_interval plus a tolerance bounding the allowed jitter. Coordinate hierarchy: - Split SampledMixin into PiecewiseMixin (gaps/overlaps logic) and RegularMixin (nominal sampling-interval marker). SampledCoordinate carries both; plain InterpCoordinate only PiecewiseMixin. - Tighten InterpCoordinate._isvalid to match exactly tie_indices + tie_values so the Coordinate factory dispatches reginterp data to RegularInterpCoordinate instead of raising. - Remove stray legacy helpers (to_dataarray/to_dict/from_dict/ to_dataset/from_dataset/from_block) that had landed on the mixin. Sampling interval: - Plain InterpCoordinate no longer exposes get_sampling_interval; it gains a private _nominal_sampling_interval plus to_regular() that builds a RegularInterpCoordinate (raising if the axis is too irregular to have a unique rate). - The module-level get_sampling_interval(da, dim) helper auto-converts via duck-typed to_regular(). Rebuild propagation: - All tie-point-rebuilding methods (_slice/_concat/simplify/__add__/ __sub__) route through a single overridable _reconstruct(data, scale, other) hook; RegularInterpCoordinate overrides it to scale and carry sampling_interval/tolerance, fixing slice/concat/add/sub/ simplify and empty construction. IO: - RegularInterpCoordinate serialises sampling_interval/tolerance as interpolation attrs (with timedelta64 encoding); reading dispatches through the Coordinate factory. Shared encode_delta/decode_delta and the unit-code tables now live in core.py. Tests cover the factory dispatch, slice/concat/add/sub/simplify, empty construction, dataset and file round-trips (numeric and datetime), to_regular jitter handling, and the auto-converting helper.
Add the RegularInterpCoordinate API reference section, swap InterpCoordinate.get_sampling_interval for to_regular in the listing, and note in the 0.2.8 release notes the new type, the PiecewiseMixin / RegularMixin split, and that plain InterpCoordinate no longer exposes get_sampling_interval.
…ailing validation InterpCoordinate.get_sampling_interval had inverted logic: it returned early when delta is not None, then tried to call .dtype on None when it wasn't set. Fixed to return None when sampling_interval is unset, applying the timedelta cast only when a value exists. from_block was passing sampling_interval through __init__ validation with tolerance=0, which failed due to floating-point rounding when computing step*(size-1)/(size-1). Since from_block constructs the tie_values directly from step, the consistency is guaranteed; fixed by assigning sampling_interval and tolerance directly to data after base construction.
Merge the regular-sampling behaviour into InterpCoordinate (carried by an optional sampling_interval/tolerance) and drop the separate RegularInterpCoordinate type and the RegularMixin marker. Replace the isinstance(coord, PiecewiseMixin) / isinstance(coord, RegularMixin) type checks with isregular() and ispiecewise() predicates on the Coordinate base, and make get_sampling_interval part of the abstract interface. Update call sites, tests, API docs, and release notes accordingly.
InterpCoordinate.from_block built tie_values[1] from the raw step while storing sampling_interval from a dtype-promoted copy of it. With a lower-precision step (e.g. a float32 SpatialSamplingInterval) the two disagreed by more than the tolerance, so the coordinate failed its own validation when rebuilt through the constructor during DataArray creation (e.g. opening OptaSense/ProdML files). Derive the endpoint from the parsed sampling_interval so the coordinate is self-consistent and survives a round-trip through its constructor.
ScalarCoordinate now implements only the thin Coordinate interface (no more stub methods raising TypeError). DenseCoordinate, InterpCoordinate, and SampledCoordinate are reparented to AxisCoordinate, which holds the full axis-mapping contract. The Coordinate.isscalar() predicate is removed; use isinstance(coord, AxisCoordinate) instead. All coordinate imports outside xdas/coordinates/ now go through from ..coordinates rather than reaching into submodules.
The piecewise gaps/overlaps/simplify contract now lives directly on AxisCoordinate, so every axis coordinate (including DenseCoordinate) supports get_split_indices, get_discontinuities, get_availabilities, and simplify. - Replace the PiecewiseMixin class with a single concrete get_split_indices on AxisCoordinate plus one abstract _split_candidates hook per subclass. - InterpCoordinate/SampledCoordinate collapse their get_split_indices into small _split_candidates implementations (no behavior change). - DenseCoordinate gains a piecewise _split_candidates (median-diff nominal spacing) and a degenerate no-op simplify; get_div_points is removed. - Drop the now-meaningless ispiecewise() predicate; concat_coords uses isinstance(out, AxisCoordinate) to decide whether to simplify.
Replace the global nominal sampling interval in InterpCoordinate._split_candidates with the sampling interval of the segment immediately left of each unit-spaced tie gap. This keeps discontinuity detection local, so a continuous coordinate that changes sampling rate is no longer skewed by rates elsewhere on the axis. Fall back to the right segment for a leading gap, or the gap itself when it is the only segment, which also fixes a TypeError on all-unit-spaced coordinates.
Replace the median-based nominal sampling interval with the exact minimax spacing: the value minimising the worst per-segment accumulated drift, which is precisely what _is_valid_sampling_interval bounds. Fold the logic into to_regular (dropping _nominal_sampling_interval), add a tolerance="auto" mode that picks the smallest valid tolerance, and skip recomputation when the coordinate is already regular.
Detect split candidates against a running reference step that follows sustained sampling-rate changes, so a continuous axis whose rate changes is no longer reported as a discontinuity while genuine gaps still are. Also make parse_scalar_delta raise on a None value when no default is available.
… rule Add _continuous_segments to encode the CF rule (den==1 = discontinuity, see section 8.3) in one place, used by both validity checking and spacing inference. Expand the class docstring with the continuous-area/discontinuity model and regularity semantics, add short docstrings to the sampling-interval helpers, and drop the misleading tolerance=None default on _is_valid_sampling_interval.
Replace the O(n^2) pairwise height matrix in to_regular's spacing inference with the upper-envelope (convex-hull trick) of the 2n lines +/-(den*si - num); its lowest vertex is the binding pair. The inner hull loop is numba-compiled, keeping prep/lexsort vectorized.
Add a docstring explaining how the tolerance bound implicitly preserves CF 8.3 structure, and extend the test suite with cases for real and soft discontinuities, multiple runs with isolated tie points, kink preservation, and datetime tie values.
`__len__` uses the documented `tie_indices[0] == 0` invariant, `_slice` drops a Python-loop in favour of a vectorised `_get_value` call, the Chebyshev-center pair is returned as `(pos_idx, neg_idx)` rather than the opaque `(i, j)`, and `_upper_envelope_min_pair` carries an explicit invariant comment on why its scan is bounded.
Douglas-Peucker can fuse a soft discontinuity into a continuous ramp; the merged segment then carries the absorbed jump on top of the original tie-value jitter. Storing `self.tolerance + tolerance` covers that worst case and degrades to `self.tolerance` for a lossless simplify (`tolerance=0`).
Previously a regular coord short-circuited to `self.copy()` whenever no `sampling_interval` was forced, silently dropping any user-supplied `tolerance` (including `tolerance="auto"`). Default each unspecified argument to the stored value instead, so an explicit override on either axis is always respected.
`_concat` is a low-level primitive: it preserves the regular contract only when both sides advertise the exact same `sampling_interval`, otherwise the merged coord is irregular. The joining tie pair is a CF discontinuity, so each side's segments validate independently and `max(tolerance)` bounds the union; raising on a mismatch was punishing a perfectly representable result. User-facing reconciliation moves up to `concat_coords`: after the usual `simplify` step, an irregular merge gets one chance to recover a single shared rate via `to_regular(tolerance=...)`, falling through unchanged when no spacing fits.
The "auto" branch picked the smallest tolerance that kept the inferred spacing valid, which silently accepted arbitrarily large drift on pathological inputs - the very opposite of what a tolerance argument is supposed to enforce. Callers that genuinely want to absorb the worst-case drift can now pass an explicit numeric tolerance instead.
Split AxisCoordinate.simplify into two opt-in stages: reduce (drop redundant tie points, default on) and regularize (acquire a nominal sampling_interval, default off). InterpCoordinate gains the promotion logic; Sampled/Dense treat regularize as a no-op. Thread reduce/regularize through concat_coords (regularize on, so the rate-recovery path keeps working) and the public concat (regularize off, preserving round-trip equality). Make to_regular/infer_regular private.
Both the time and frequency output coordinates now use coord_cls.from_block() where coord_cls mirrors the input dimension's coordinate type, rather than falling back to a generic InterpCoordinate dict. This ensures sampling_interval is properly preserved for SampledCoordinate inputs.
Provides a minimal DataArray for use in tests and doctests. Defaults to 100 × 10 (100 Hz, 10 m spacing → 1 s × 100 m). Accepts a step argument (scalar or per-dimension tuple); float steps on the datetime dimension are auto-converted to timedelta64[ns].
Replace the standalone get_sampling_interval(da, dim) calls in signal.py with da.coords[dim].get_sampling_interval(), propagate sampling_interval through UpSample and resample_poly coordinate reconstruction, and update tests to use the new coordinate API.
Promote to_regular to the public AxisCoordinate interface: InterpCoordinate enforces or infers a spacing, SampledCoordinate validates and copies, and DenseCoordinate converts to a regular InterpCoordinate. isregular() moves to the Coordinate base (False for scalars) so the predicate exists on the whole hierarchy. Regularity now means "carries a declared sampling_interval". Accordingly DenseCoordinate.get_sampling_interval returns None instead of the end-to-end average, which was vacuously "regular" for any dense axis and fed meaningless rates to signal processing on jittery data. The module-level get_sampling_interval becomes the single choke point for signal routines (signal.py stops open-coding the check). Data written by earlier versions carries no declared rate, so rather than breaking every existing archive it falls back to inferring one and emits a FutureWarning stating the inferred value, the tolerance it requires and the migration path. It raises only when no spacing can be inferred at all. Also fix from_block for sizes below two, which built invalid tie indices.
Treat tolerance as a property of the coordinate rather than a per-call parameter: simplify(tolerance=None) now spends the coordinate's own declared jitter instead of a zero-like default, and a regular coordinate keeps its tolerance through a reduce pass unless the fused values no longer validate, in which case it widens by the spent budget only. Operations that derive a new rate declare the error they introduce: UpSample records the truncation residue of delta // factor on top of the inherited jitter. Chunk seams then land within tolerance of the nominal grid, so chunked and unchunked pipelines produce equal coordinates again. Align concat_coords defaults with concat (tolerance=None, regularize=False); regular inputs stay regular through concatenation, so promotion is only needed for irregular ones and remains opt-in. Bag compatibility checks use the coordinate-level primitive so an irregular chunk yields a CompatibilityError rather than a TypeError.
Scanners know the acquisition rate, so build coordinates that declare it: prodml and terra15 derive it from the file's own timestamps, the ASN ZMQ subscriber from its header, and from_stream via from_block at nanosecond resolution so a to_stream round trip preserves the coordinate. Per-file tolerance stays zero; cross-file jitter is reconciled at concat time. The FFT functions likewise emit regular frequency and signal axes, without which an fft/ifft round trip would leave the result unusable by any further signal processing.
Add a "Regular coordinates" section to the interpolated-coordinates guide, list to_regular and the module-level get_sampling_interval in the API reference, give xdas.testing its own page, and drop the stale synthetics.dummy entries left by the move to xdas.testing. Rewrite the 0.2.8 release notes as a net diff from 0.2.7 rather than a log of the development history: the sampling-interval change and its transition shim are stated once under Deprecations, and API surface churn that no ordinary user code touches sits under Refactoring. docs/plan_regular_coordinates.md records the design decisions behind the change and can be dropped before merging.
Replace hand-rolled DataArray construction and wavelet_wavefronts payloads with xd.testing.dummy wherever the test only cares about shapes, coordinate positions and round-tripping rather than the signal itself. Coordinate, trigger, picking and StreamWriter tests keep their explicit fixtures since the data values or datetime literals are load-bearing there.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #78 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 42 44 +2
Lines 4595 4739 +144
Branches 702 743 +41
==========================================
+ Hits 4595 4739 +144 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Makes the sampling interval a declared property of a coordinate instead of something inferred on the fly, and threads that notion through the coordinate stack, the IO engines and the signal-processing routines.
Why
get_sampling_intervalused to return the end-to-end average spacing of any coordinate. On a jittery or gappy axis that is a silently wrong number, and it was fed straight into filters, FFTs and resampling. There was also no way for a coordinate to state "my nominal rate is 100 Hz, values may wobble by up to 2 ns", so chunked and unchunked pipelines produced coordinates that compared unequal.What changes
InterpCoordinatecan carry a nominalsampling_intervalplus atolerancebounding the allowed jitter.isregular()/get_sampling_interval()query it; the newto_regular()— available on everyAxisCoordinate— promotes an irregular coordinate, inferring the spacing when not given and raising on genuinely irregular axes.DenseCoordinateis never regular: it converts instead of averaging.simplify(tolerance=None)andconcatspend the coordinate's own declared tolerance; operations that derive a new rate (e.g.UpSample) record their rounding residue in it. Chunk seams therefore fuse away on concatenation and chunked/unchunked results are equal again.asn,prodml,terra15andminiseedengines, the ASN ZMQ subscriber,from_stream, and the FFT/STFT outputs all emit regular coordinates, so signal processing composes end to end without manual rate bookkeeping.Coordinateis a proper ABC; a newAxisCoordinateABC holds the axis-mapping contract shared by dense/interpolated/sampled coordinates. Theis*predicates,DefaultCoordinate,to_dict/from_dict,decimate,from_arrayandget_div_pointsare gone; several implementation-detail methods are now underscore-private.xdas.testing.dummy. A new public helper builds a minimalDataArraywith regular coordinates of a chosen type, shape, step and dtype. It gives fixtures a one-line way to declare a rate, which is what the stricter contract now asks of every caller.Backward compatibility
Data written by earlier versions carries no declared rate, so requiring one would break every existing archive. For one deprecation cycle an irregular coordinate still works: the rate is inferred and a
FutureWarningreports the inferred value, the tolerance it implies, and the one-line fix (da[dim] = da[dim].to_regular(tolerance=...)). A later release will raise instead.FutureWarningrather thanDeprecationWarningso end users actually see it.Review notes
docs/plan_regular_coordinates.md; that file is meant to be dropped before merging — it is committed only to make the reasoning reviewable.test_upsample,test_stream, twoxdas/io/asn.pydoctests); they are fixed by the third commit, and no commit introduces a new failure.xdas.testing.dummy(14 files, −277 net lines) — coordinate, trigger, picking andStreamWritertests keep their explicit fixtures, because there the data values or the datetime literals are load-bearing. Test count and per-file coverage are unchanged by that commit.DownSampledrops one sample when the axis length is not a multiple of the decimation factor (100 samples atq=3→ 33 chunked vs 34 monolithic; 102 and 300 agree). The old fixture happened to be a multiple, so it never showed.test_downsamplenow pins a multiple length with a comment saying why.Verification
uv run pytest --cov→ 867 passed, 100 % coverage (statements and branches).ruff check/ruff formatclean.make htmlbuilds with only the 2 warnings that already existed ondev.