Add the DASDAE inventory model layer - #843
Conversation
Implements the DASDAE inventory specification (dasdae.github.io/inventory) core model in dascore.core.inventory: - All spec objects as immutable pydantic models: Inventory, Network, Station/Channel/Response, FiberArray, Acquisition, DistanceMap, OpticalPath, optical components (discriminated union), Geometry, CouplingCondition, OpticalPathAnnotation, resources, CRS - Track-kind rules: components tile the path span; geometry and coupling are function tracks (partial coverage legal, overlap raises); annotations overlap freely - Half-open UTC validity epochs (NaT = ongoing); one optical path per (fiber array, location code) at a time; epoch overlap raises at validate(), gaps and abutting epochs are legal - Channel resolution: affine (start_distance + spatial_interval) XOR measured DistanceMap (channel or instrument_distance input axis, single-point maps take slope from spatial_interval) - data_source_id + time resolution to (network, array, acquisition, path); code charset enforcement (letters/digits/'-', blank location allowed) - Path operations preserving absolute distances: select, split_at, reverse (rewrites all tracks), concatenation - Inventory.replace (corrections), YAML round-trip, dc.inventory() Tests include a conformance suite driven by a vendored copy of the spec YAML, so field drift from the specification fails loudly.
- Open-ended epoch overlap uses null-aware comparisons instead of finite sentinel dates - A set end_time must follow start_time (construction-time check) - Station/Channel accept dynamic coordinate-label fields per the spec; get_coordinates(crs) validates them against the CRS and agree-or-raises against an explicit coordinates tuple - replace() requires the replacement to match the replaced type - Terminal zero-length components survive full selections and splits - Resource dict keys must agree with resource_ids; duplicate ids raise - Geometry coordinate points require nonzero dimensionality Adds regression tests for each finding.
Coordinates are stored only on the canonical axes; labels such as latitude/longitude/elevation/easting/northing are resolvable aliases from a controlled vocabulary whose meaning the inventory CRS declares (CoordinateReferenceSystem.axis_index). Aliases the CRS does not define raise. Station/Channel return to extra="forbid" — no dynamic coordinate fields, so typos fail at construction and coordinates have exactly one stored spelling. Also retire the vendored spec conformance suite: the design phase is over and this implementation is now the source of truth for the model.
Resource-valued fields (container, specification, interrogator, otdr_traces) accept an object or a resource_id string. At Inventory construction a single sweep moves inline resource objects into the resources pool (nested resources included) and rewrites the fields to id references, so the canonical form is flat: one copy per resource, shared by id. Two inline definitions of one id must be equal and id references must resolve, or construction raises. replace() on a resource becomes a single-site pool correction and requires the resource_id to stay stable so references never dangle. Adds Inventory.get_resource.
…ation Correctness: - new() on inventory models dumps all fields so union discriminators and normalizer-installed values survive reconstruction; the normalizer also records its fields as set - sensible_model_equals compares recursively with null-aware semantics, so nested NaT epochs no longer break equality, replace() lookup, or round-trip comparison - replace() re-runs validators (normalizing inline resources, rejecting dangling references), reaches channels, and documents matching against normalized objects - Keyless dict resources adopt their pool key as resource_id; network check() rejects duplicate same-code overlapping arrays/stations; path check() rejects mixed-dimensionality geometry - inventory()/from_yaml dispatch like spool(): Inventory passthrough, clear errors for missing files and unhandled types Conventions: - Rename tree validation to check(): the instance method shadowed pydantic's deprecated BaseModel.validate classmethod and failed ty - Use spelled-out Literals (DataType, DataCategory, CouplingType via get_args) per house style; branch is ty-clean - Export Inventory from dascore.core; add doctested example Duplication (per review): - Eight per-class code validators collapse into CodeStr/LocationCodeStr Annotated aliases; ResourceIdStr/NotesStr aliases replace repeated field boilerplate - _IntervalModel base unifies CouplingCondition/OpticalPathAnnotation; Geometry.distance_span renamed interval for uniform track access - One _overlapping_epochs helper replaces three pairwise-loop copies; one normalize() mechanism replaces three per-type closures - Remove dead coupling_at; fold resolve() repetition into exactly_one
…ness - replace() reaches path track items (components, geometry, coupling, annotations); its docstring states the addressable scope precisely - Normalized string references must resolve to the field's allowed resource type, not merely exist in the pool - Recursive equality requires nulls to match positionally instead of treating a null on either side as equal (regression in the previous fix affecting shared PatchAttrs machinery) - Distance-bearing values must be finite (interval fields, component lengths, path origins, geometry and distance-map control points) - Apply ruff format to the changed files
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a DASDAE inventory model with typed metadata, spatial and temporal validation, optical-path operations, resource management, resolution, YAML serialization, factory loading, and public exports. ChangesInventory model
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #843 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 170 176 +6
Lines 18374 19359 +985
==========================================
+ Hits 18374 19359 +985
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1916f6e0dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| default_factory=CreationInfo, | ||
| description="QuakeML-style creation and update metadata.", | ||
| ) | ||
| resources: dict[str, _Resource] = Field( |
There was a problem hiding this comment.
Although every inventory model is documented as immutable and resource corrections are routed through replace(), Pydantic's frozen configuration is shallow, so callers can still execute operations such as inventory.resources.clear() or assign a different resource directly. This bypasses normalization and type checks and can leave path components holding dangling or incorrectly typed resource IDs; expose an immutable mapping or a defensive view instead.
Useful? React with 👍 / 👎.
| spatial_interval: float | None = Field( | ||
| default=None, |
There was a problem hiding this comment.
When an acquisition is constructed with spatial_interval=np.nan or infinity, validation accepts it because this field has no finite-value constraint, and channel_to_distance() only checks for None. Every resolved channel distance then becomes NaN or infinite (and the same value is used as a single-point DistanceMap slope), silently producing unusable coordinates instead of rejecting invalid metadata.
Useful? React with 👍 / 👎.
| for net in self.networks: | ||
| net.check() |
There was a problem hiding this comment.
For an inventory whose CRS declares three axes, a path containing two-dimensional geometry—or a station/channel with a mismatched coordinate tuple—still passes check(), because nested checks only compare geometry segments with each other. This leaves axis_index("z") reporting index 2 while coordinates_at() returns only two columns, so consumers interpreting coordinates through the inventory-wide CRS can index nonexistent axes or assign incorrect meanings.
Useful? React with 👍 / 👎.
| if item == old: | ||
| out.append(new) | ||
| replaced += 1 |
There was a problem hiding this comment.
When a path contains multiple equal components, replace() replaces every occurrence even if old is the actual object retrieved from just one position. This is common for repeated default connectors or terminators, so correcting one component's metadata can silently rewrite unrelated components elsewhere in the inventory; prefer an identity match when available and reject an ambiguous equality-only match rather than applying it to all matches.
Useful? React with 👍 / 👎.
| channels: tuple[Channel, ...] = Field( | ||
| default=(), description="Channels associated with this station." | ||
| ) |
There was a problem hiding this comment.
A station can contain multiple channels with the same (location_code, code) and overlapping validity epochs, and the whole-inventory check() still succeeds because Station has no channel epoch validation. Since those fields form the channel's stream identity, an identifier plus time cannot distinguish such entries; enforce the same overlap rule used for acquisition identities.
Useful? React with 👍 / 👎.
| return self.model_copy( | ||
| update={ |
There was a problem hiding this comment.
Adding two paths always retains the left operand's location_code, start_time, and end_time while incorporating all components and tracks from the right operand. If paths from different lineages or validity epochs are combined, the result therefore advertises the left metadata and can resolve right-hand physical metadata at times or locations where it was never valid; require compatible path identity and epoch fields or define the combined metadata explicitly.
Useful? React with 👍 / 👎.
- PyYAML is not a dascore dependency; to_yaml/from_yaml now use optional_import so environments without it (wasm, free-thread, ty) import cleanly, and yaml-dependent tests importorskip - The dc.inventory doctest no longer requires yaml - Cover every remaining new-code branch (codecov requires 100% patch coverage); remove one mathematically unreachable degenerate-clip check in geometry selection
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_core/test_inventory.py (2)
412-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the concrete exception type.
OpticalPath.selectraisesParameterError.pytest.raises(Exception, ...)also passes if the code raises an unrelated error whose message happens to match, and it hides a future change of exception type.♻️ Proposed change
+from dascore.exceptions import InvalidInventoryError, ParameterErrordef test_empty_selection_raises(self, path): """Empty selection raises.""" - with pytest.raises(Exception, match="Empty distance selection"): + with pytest.raises(ParameterError, match="Empty distance selection"): path.select(distance=(300.0, 400.0))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_core/test_inventory.py` around lines 412 - 415, Update test_empty_selection_raises to import and assert the concrete ParameterError type raised by OpticalPath.select instead of the broad Exception class, while preserving the existing “Empty distance selection” message match.
477-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName test classes after behavior, not the review that produced them.
TestReviewRegressions,TestInternalReviewRegressions, andTestCodexReviewRegressionsgroup unrelated cases by their origin. A reader cannot tell which rule each class covers, and new tests have no obvious home. Consider folding these cases into the behavior-named classes that already exist, such asTestEpochs,TestResourcePool, andTestPathOperations.Also applies to: 685-686, 808-809
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_core/test_inventory.py` around lines 477 - 478, Rename or move the review-origin test classes TestReviewRegressions, TestInternalReviewRegressions, and TestCodexReviewRegressions into existing behavior-focused classes such as TestEpochs, TestResourcePool, and TestPathOperations. Group each test with the class matching the behavior it verifies, and remove the review-based grouping names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/core/inventory.py`:
- Around line 871-873: Add strict=True to the zip call in OpticalPath.select at
dascore/core/inventory.py lines 871-873, and to the zip(val1, val2) call in
_values_equal at dascore/utils/models.py lines 115-118 after its existing length
check; no other changes are needed.
- Around line 798-812: Update coordinates_at to validate that all geometry
segments have the same coordinate dimensionality before allocating or assigning
into out, reusing the existing dimensionality-check logic and
InvalidInventoryError message from OpticalPath.check. Ensure mixed 2-D/3-D paths
raise InvalidInventoryError instead of reaching the broadcast assignment.
---
Nitpick comments:
In `@tests/test_core/test_inventory.py`:
- Around line 412-415: Update test_empty_selection_raises to import and assert
the concrete ParameterError type raised by OpticalPath.select instead of the
broad Exception class, while preserving the existing “Empty distance selection”
message match.
- Around line 477-478: Rename or move the review-origin test classes
TestReviewRegressions, TestInternalReviewRegressions, and
TestCodexReviewRegressions into existing behavior-focused classes such as
TestEpochs, TestResourcePool, and TestPathOperations. Group each test with the
class matching the behavior it verifies, and remove the review-based grouping
names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5a44c9d-03c8-4f31-9c8b-29ae1552ba98
📒 Files selected for processing (6)
dascore/__init__.pydascore/core/__init__.pydascore/core/inventory.pydascore/exceptions.pydascore/utils/models.pytests/test_core/test_inventory.py
The serialization type tag is now declared through a _type_tag helper: hidden from repr, defaulting to the class name, and constrained by its Literal so it can never be set to anything else. Users never interact with it; it exists for serialized YAML/JSON, where it drives union dispatch and the authoring format's type declaration.
Every inventory object now carries the two attachment points for unmodeled information: notes (free prose) and extra_fields (typed key-values, e.g. round-tripped vendor or external-format metadata). This fills the previous gaps (Acquisition and OpticalPath had no notes; only two classes had extra_fields) and removes the per-class declarations. Cable drops owner and serial_number (notes-grade custody trivia, matching the Enclosure trim); Interrogator keeps serial_number as the physical instrument's identity. to_yaml now prunes empty strings, mappings, and sequences — lossless, since all model fields default to empty — so serialized files get cleaner rather than wider. User values inside extra_fields are kept verbatim.
Aerial (pole-suspended) fiber joins the coupling vocabulary. Interval items (coupling conditions and annotations, via their shared base) may now have optical_length == 0: a point marker documenting a location — a clamp, a labeled spot — that covers no distance and therefore never participates in coverage, enrichment, or function-track overlap checks, so a clamp point inside a trenched span is representable.
Coupling conditions and annotations now cover [start_distance, end_distance) instead of start plus length. Field data reads off OTDR and interrogator displays as two absolute positions (transcribing a length invites arithmetic errors), and start/end matches the model's one interval idiom everywhere: start_time/end_time epochs and the path's own start_distance/end_distance span. optical_length remains as a derived property; a point marker is start == end.
Every path component carries loss_db (one-way transmission loss) and reflectance_db (return loss) — the two quantities an OTDR trace shows per event — replacing insertion_loss, the attenuation pair, and the terminator-only reflectance. Each value pairs with the measurement record that produced it: the new OpticalMeasurement shareable resource captures the conditions (method, time, wavelength, pulse width, direction, trace file) once, and every number from the same run references the same pooled record; a datasheet claim is an honest record too (method="datasheet"). Multi-wavelength values are equal-length tuples paired elementwise with their measurements. FiberSegment keeps the familiar dB/km reading as a derived property and drops the engineered-backscatter datasheet fields (specification links the datasheet). OpticalPath.otdr_traces becomes measurements, holding OpticalMeasurement references.
units becomes a tuple paired one-per-axis with coordinate_labels (the old single string claimed elevation in degrees under the default CRS). A wkt field carries WKT2 definitions for frames an authority code cannot describe — local engineering grids, derived grids, compound horizontal+vertical CRSs — which is what GIS tools need to actually interpret them; registry CRSs leave it empty, since the EPSG code is already the complete definition.
One uniform prose field on the InventoryModel base, named to match StationXML's Description element; Network's bespoke description field is now the inherited one.
Network becomes a TimeRangedModel (FDSN networks carry start/end dates); Channel gains optional azimuth, dip, and depth so imported point-sensor channels stay interpretable.
Scheme-prefixed URI strings (doi:, ark:, urn:) mirroring StationXML's Identifier element on the levels that plausibly get cited.
| is geographic WGS84 3D (EPSG:4979); override it only for exceptional | ||
| frames such as mines or laboratories. |
There was a problem hiding this comment.
We dont need the guidelines; we are all adults here ;)
| (``network.fiber_array.location.acquisition``) which, together with time, | ||
| resolves against an inventory. | ||
|
|
||
| Key model rules (see the DASDAE inventory specification): |
There was a problem hiding this comment.
we can remove the reference to the spec; remember the goal is an implementation, not a spec.
| The inventory extends the StationXML concept with first-class support for | ||
| fiber-optic arrays. It describes the physical optical path (fiber, connectors, | ||
| splices), the geometry, coupling, and annotation tracks along optical | ||
| distance, and the interrogator configurations (acquisitions) that produced | ||
| DAS patches. Patches carry a ``data_source_id`` | ||
| (``network.fiber_array.location.acquisition``) which, together with time, | ||
| resolves against an inventory. | ||
|
|
||
| Key model rules (see the DASDAE inventory specification): | ||
|
|
||
| - Validity intervals are half-open ``[start, end)`` in UTC; an unset (NaT) | ||
| end time means ongoing. The outermost endpoint of a coverage domain is | ||
| included. | ||
| - No more than one ``OpticalPath`` per ``(FiberArray, location_code)`` is | ||
| valid at a given time. | ||
| - Optical components are the tiling track: they cover the whole path exactly | ||
| once. Geometry and coupling are function tracks: coverage may be partial | ||
| (uncovered distance is undefined), overlap raises. Annotations overlap | ||
| freely. | ||
| - Codes use letters, digits, and ``-``; ``.`` is the ``data_source_id`` | ||
| separator. All codes are non-empty except ``location_code``. | ||
| - ``Acquisition`` maps channels onto path distance through either the affine | ||
| form (``start_distance`` + ``spatial_interval``) or a measured | ||
| ``DistanceMap``; the two are mutually exclusive. | ||
| """ |
There was a problem hiding this comment.
Let's make this much more concise. Mostly, such info should live in the appropriate class. You can leave just a simple overview here, not detailed rules. Such rules will be included in a dedicated note in a future PR.
| def _is_strictly_increasing(values) -> bool: | ||
| """Return True if a sequence is strictly increasing.""" | ||
| arr = np.asarray(values, dtype=float) | ||
| return bool(len(arr) < 2 or np.all(np.diff(arr) > 0)) |
There was a problem hiding this comment.
This smells like a general util that shouldnt live here (there might already be one in utils).
| "from the controlled coordinate vocabulary." | ||
| ), | ||
| ) | ||
| units: str = Field(default="degree", description="Coordinate units when known.") |
There was a problem hiding this comment.
This wouldnt apply to elevation. Maybe this should be a tuple (x_units, y_unit, z_unit) instead?
| class Enclosure(InventoryModel): | ||
| """Physical housing, pipe, duct, conduit, or carrier resource.""" | ||
|
|
||
| type: Literal["Enclosure"] = "Enclosure" |
There was a problem hiding this comment.
Do we need other words? You list some in the docstring but dont support specifying that type here?
| least two strictly increasing optical distances, each paired with the | ||
| coordinate at that point (interpreted using the inventory CRS). Coverage | ||
| is the half-open span of the array; there is no separate length field. A | ||
| coil is a segment whose coordinates repeat while distance advances. |
There was a problem hiding this comment.
"A coil, or other 'clump'" can be represented... "
|
|
||
|
|
||
| class Interrogator(InventoryModel): | ||
| """DAS interrogator unit used for data collection.""" |
There was a problem hiding this comment.
Should say DFOS. We want DASCore to work for more than just DAS
| material: str = Field(default="", description="Material of the enclosure.") | ||
| manufacturer: str = Field(default="", description="Manufacturer name.") | ||
| model: str = Field(default="", description="Model name.") | ||
| inner_diameter: float | None = Field( |
There was a problem hiding this comment.
Maybe we just call it diameter, since now we don't have an outer diameter?
| return True | ||
|
|
||
|
|
||
| def _values_equal(val1, val2) -> bool: |
There was a problem hiding this comment.
Doesnt the DASCoreBaseModel already have eq that can be used here?
Annotations become group/value pairs whose value type decides how they enrich: boolean groups state membership and may overlap, string and numeric groups are single valued and may not. FiberSegment gains refractive_index and renames fiber_index/color to fiber_number and fiber_color. The coordinate vocabulary moves into the type, gains depth, and leaves only a uniqueness check behind; the module docstring keeps an overview and lets each object document its own rules; the strict monotonicity test moves to utils.misc, shared with coords. Also addresses review findings: non-finite affine parameters, duplicate channel identities, concatenation across lineages or epochs, ambiguous replacements, resources addressed by id, coordinate widths against the CRS, and mixed-dimension geometry in coordinates_at.
Annotation values keep their Python type (a numpy mask element is a flag, not the number one) and must be finite; an empty value is no longer pruned on serialization, where it would reload as the boolean default and change its group's kind. Point markers survive select and split_at, as they already did for components. Network epochs are now usable: duplicate codes are judged by overlapping epochs and resolve honors the requested time. Coordinates must be finite, a CRS declares one to three axes to match the canonical (x, y, z) storage, and channel azimuth and dip enforce the ranges they document.
…cuit Every physical scalar rejects nan and inf, the concatenation precondition is documented on __add__, and the ambiguous-replacement message names a field that track items actually have. is_strictly_monotonic answers ascending input in one pass again and treats non-1D input as unordered rather than raising.
Reading a document asks whether it is a valid inventory, so from_yaml checks before returning; in-memory construction stays unchecked so an inventory can still be assembled a piece at a time.
1 participant
Description
This PR adds the core model layer of the DASDAE inventory — a StationXML-style, fiber-native metadata model for DFOS observing systems — as
dascore.core.inventory. It is the first of several planned PRs (patch enrichment, spool integration, and the directory authoring loader will follow separately).What it provides:
Inventory,Network,Station/Channel/Response,FiberArray,Acquisition,DistanceMap,OpticalPathwith its component/geometry/coupling/annotation tracks, and shareable resources (Interrogator,Cable,Enclosure,ExternalResource).[start, end), NaT = ongoing): abutting epochs are legal, overlapping same-code epochs raise atcheck(), and no more than one optical path per(fiber array, location code)is valid at a time.start_distance+spatial_interval) or a measuredDistanceMap(channel or instrument-distance input axis), mutually exclusive.data_source_id+ time resolution (Inventory.resolve), code charset enforcement, coordinates stored on canonical(x, y, z)axes with CRS-resolved label aliases.resourcespool withresource_idreferences: inline objects are hoisted at construction, references are type-checked, andreplace()corrections on a resource are single-site.select,split_at,reverse, concatenation), YAML round-trip, anddc.inventory().Design background: the model was developed through an iterative spec process (with adversarial design reviews) in the DASDAE/inventory repo; that content will migrate into DASCore's docs in a follow-up. The implementation went through two internal review rounds plus two external review rounds; all findings are covered by regression tests in
tests/test_core/test_inventory.py(82 tests).Notable shared-machinery changes:
dascore/utils/models.pygainsInventoryModel/TimeRangedModelbases, andsensible_model_equalsnow compares nested structures recursively with positional null-matching (previously nested NaT/NaN values made structurally identical models compare unequal).Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Tests