Add the inventory patch surface: data_source_id, reader vocabulary, and enrich - #855
Open
d-chambers wants to merge 11 commits into
Open
Add the inventory patch surface: data_source_id, reader vocabulary, and enrich#855d-chambers wants to merge 11 commits into
d-chambers wants to merge 11 commits into
Conversation
Patch attrs had no story about who owns which field. Identity was spread over network/station/instrument_id/acquisition_id, every reader spelled the observing system's facts its own way, some shipped a companion attr naming a value's units, and a few put the file path in the patch. None of that can meet an inventory: two spellings of gauge length are two attrs, and nothing downstream can reconcile them. Sort every attr into one of four kinds and let the kind decide the name and the owner. Identity becomes the single data_source_id field holding network.fiber_array.location.acquisition; component queries are wildcard matches on it. station is not reused because DAS MiniSEED already spends it on a per-sampling-point identity. Observing-system facts take the inventory's own names, listed in INVENTORY_ATTRS and tested against the models they mirror, with nested facts dotted (interrogator.serial_number). Those attrs carry fixed units, so readers convert at the parse boundary through convert_attr_units and the *_units companions are gone. Storage provenance keeps the source_ prefix everywhere, including the contents and index columns, and leaves patch attrs entirely: it says where the bytes live, and a patch merged from three files has no single answer. A conformance test holds every shipped reader to the vocabulary, so a format added next year cannot quietly reinvent channel_spacing. Extra attrs are still allowed; the test is a review checkpoint, not a runtime rule. Breaking: the removed attrs fields, the reader respellings, the *_units removal, and the source_* column rename are all in the changelog.
An inventory that a patch cannot be joined to is a document, not metadata. enrich is that join: the patch's data_source_id and time resolve one acquisition and optical path, the acquisition's channel map places each channel on the path axis, and the path's tracks project onto the patch. The rules follow from who owns what. Blanket attrs=True copies the observing-system facts under the inventory's own names and leaves the data trio alone, since processing maintains those; naming one restores the as-acquired value. Disagreements go through conflicts, reusing chunking's flag and vocabulary, so a header contradicting the resolved acquisition can raise instead of being silently replaced -- usually the sign that the data_source_id resolved to the wrong place. A patch straddling an epoch boundary raises rather than picking one side, because acquisition metadata is scalar per patch. Which patch coordinate the channel map applies to is the map's to declare: channel numbers for the affine form and a channel-axis distance map, interrogator meters for an instrument_distance map. Guessing would be wrong by a factor of the channel spacing, so a patch without the declared coordinate raises and says which map form fits it. Long argument reprs are now truncated in patch history; without that every enriched patch carried a full inventory dump in its history.
Calling enrich on every extracted patch by hand is the kind of step that gets forgotten halfway through an analysis, and the spool already knows which patches it is about to hand out. attach_inventory holds the inventory and enriches on extraction, so metadata arrives with the data. The inventory joins the spool's equality state: two spools over the same rows yield different patches when one is attached, so they are not the same spool. This is deliberately only the extraction hook. Resolving the index against the inventory, subdividing it at epoch boundaries, and selecting on inventory tracks are phase 3 and are called out as absent in the method's own documentation.
Four reviews (one Codex, three adversarial) against the three phase-2 commits. Every finding below was reproduced before it was fixed. Data that was silently wrong: - Febus A1 states its pulse as a length in meters, which the deleted units companion had recorded correctly; converting it as nanoseconds made it wrong by eight orders of magnitude. It keeps terra15's pulse_length name for the same quantity. - Silixa's Carina variant reads the same nanosecond header key through a second code path, which the conversion missed, so 110 ns became 110 seconds. - Neubrex spells the units key in the singular, so its conversion never ran and any file stating cm would have passed through as meters. - A file which declares units that cannot be used has a value of unknown scale. Keeping the number passed "5 s" off as 5 meters; the value is now dropped with a warning. - TDMS filtered its parsed header through PatchAttrs.model_fields, which no dotted name can be in, so it dropped the interrogator serial it had just read. Failures which only surfaced at load: - An attr named path, file_format, or file_version stopped being reserved when the flat columns were renamed, so it was splatted into dc.read beside the row's own value and every trimmed read of that spool raised TypeError. - A hive path stamping an incomplete data_source_id indexed cleanly and then failed at every patch access, far from the directory that caused it. Refuse it at indexing instead. Enrichment: - Projected coordinates now carry the units the CRS and the model state. - Re-enriching is a refresh, coordinates included: an identical coordinate is not a collision, but a contradicting one still is. - NaN is how a reader spells an unknown number, so it is filled rather than treated as a value which disagrees. Previously conflicts="drop" deleted the very value enrichment exists to supply. - sample_rate and spatial_interval are maintained by processing, so blanket enrichment no longer restores stale ones; naming one still does. A patch decimated in time kept its as-acquired sample rate. - Coverage runs end where they end: a track interval elsewhere on the path no longer changes which channels an earlier interval covers. - A geometry-less path, an unset or misspelled track field, and a multi-valued component field now report through on_missing or raise a named error instead of returning silent NaN or a numpy broadcast error. - A union of spools carries an attached inventory instead of dropping it, and two different attachments raise.
An acquisition had two ways to place its channels on the path and a validator to keep authors from using both. The affine form (start_distance + spatial_interval) mapped channel numbers, which no DASCore reader produces: of the 38 example files, 36 carry meters and none carry channels, most with a spacing that is not one and an origin that is not zero. So the form that looked simple was the one nobody could use, and the rule policing the pair existed only because there were two. A single control point says everything the affine form said -- an origin, with spatial_interval as its slope on the channel axis and one meter of path per interrogator meter on the other -- and it extrapolates, where a two-point map goes undefined past its ends. So the affine form and its exclusivity rule are gone. With one mechanism left, "exactly one input axis" was also the wrong rule for the map itself. It now states at least one, and may state both, so one set of control points can be written in whichever coordinates were measured and serve patches whose axes differ. The patch decides which axis is read. A patch carrying more than one of them must agree with the map about all of them: two coordinates which place the same channel in different places mean the patch contradicts the map, and picking one would answer a question the data disputes. An acquisition written against the removed field now says what replaced it instead of raising pydantic's "extra inputs are not permitted".
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds inventory-based patch enrichment and spool attachment. It consolidates patch identity into ChangesInventory and metadata integration
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Reviews of the two-axis relaxation. A map stating both axes describes one interrogator, which samples at a fixed spacing, so axes implying a spacing that varies along the fiber now raise: they describe no instrument, and reading such a map on one axis contradicts reading it on the other. The axis argument was an unvalidated getattr, so axis="distance" -- the name of the patch coordinate the instrument axis is read from, and so the likeliest wrong value -- silently interpolated distance onto distance and returned the input unchanged. An axis the map cannot be read on (a channel axis with no spacing) no longer vetoes the axes that can, and the failure that results names every reason rather than the first. When two coordinates have different lengths the message says so instead of reporting two equal numbers as a disagreement, and the agreement tolerance is absolute: a relative one widens to a whole channel tens of kilometers down the fiber, which is exactly where a disagreement matters.
![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: 2eb7f151ea
ℹ️ 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".
| "inventory to the combined spool instead." | ||
| ) | ||
| raise InvalidSpoolError(msg) | ||
| return attached[0] |
There was a problem hiding this comment.
When only one operand has an attached inventory, returning that attachment causes the combined spool's _maybe_enrich path to apply it to patches from both operands. If the unattached operand contains another data source, accessing those patches will either fail resolution or silently enrich them with unrelated metadata; the union must either retain attachment state per operand or reject mixed attached/unattached nonempty spools.
Useful? React with 👍 / 👎.
Comment on lines
+682
to
+683
| new._inventory = inventory | ||
| new._enrich_kwargs = dict(kwargs) if enrich else None |
There was a problem hiding this comment.
When an inventory is attached to a root created by Spool.from_file, calling update() is permitted but its single-file branch reconstructs the result with self.from_file(...), restoring _inventory and _enrich_kwargs to their class defaults. The returned spool therefore stops enriching patches after a rescan, unlike directory and in-memory roots; the attachment state needs to be copied to the refreshed file spool.
Useful? React with 👍 / 👎.
Comment on lines
+320
to
+321
| for name, _, distances in resolved[1:]: | ||
| if not _distances_agree(first[2], distances): |
There was a problem hiding this comment.
When a patch carries both map-supported coordinates on different one-dimensional dimensions, this loop discards each later dimension and compares only the mapped value arrays. If the dimensions happen to have equal lengths and values, enrichment accepts the ambiguous patch and attaches every projected geometry or annotation coordinate to the first dimension, potentially labeling the wrong axis; all resolved map coordinates should be required to share the same dimension.
Useful? React with 👍 / 👎.
Comment on lines
+717
to
+720
| axis = self.axes[0] if axis is None else axis | ||
| out = getattr(self, axis, None) | ||
| if out is None: | ||
| msg = f"This DistanceMap is not written in {axis!r}; it has {self.axes}." |
There was a problem hiding this comment.
When a caller passes an existing non-input attribute such as axis="distance", getattr returns the map's target-distance control points, so this check accepts them as an input axis. map_to_distance can then silently use the output distances as its source coordinate and return an incorrect identity-like calibration; reject every explicit axis that is not present in self.axes before reading the attribute.
Useful? React with 👍 / 👎.
Comment on lines
+361
to
+366
| if isinstance(value, tuple | list): | ||
| msg = ( | ||
| f"Cannot project the multi-valued {value!r} onto channels; " | ||
| "a coordinate holds one value per channel." | ||
| ) | ||
| raise PatchError(msg) |
There was a problem hiding this comment.
When a qualified request targets a populated mapping field such as coupling.extra_fields, the value bypasses this multi-value guard because only tuples and lists are recognized. _annotation_kind then classifies the dict as numeric and the final float conversion raises a raw TypeError, even though extra_fields is a legitimate field on every inventory track model; mapping and other non-scalar values should be rejected with the same PatchError as the already-handled multi-valued fields.
Useful? React with 👍 / 👎.
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
dascore/core/attrs.py (1)
88-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rejecting the removed identity keys explicitly.
extra="allow"meansPatchAttrs(network="XX", station="S1")still constructs. The values become inert extra attrs, and grouping, merging, and naming ignore them silently.Acquisition._reject_start_distanceindascore/core/inventory.pygives the analogous removal an explicit migration message. Adding the same treatment here would surface stale reader and user code at construction time.♻️ Proposed migration guard
+_REMOVED_IDENTITY_FIELDS = ("network", "station", "instrument_id", "acquisition_id") + + class PatchAttrs(DascoreBaseModel):`@model_validator`(mode="before") `@classmethod` def reject_coordinate_attributes(cls, data: Any) -> Any: """Reject nested coord payloads and ignore structural dims input.""" if not isinstance(data, Mapping): return data data = dict(data) if "coords" in data and not isinstance(data["coords"], str): msg = "PatchAttrs no longer accepts coordinate metadata. Received: coords." raise ValueError(msg) + if removed := sorted(set(data) & set(_REMOVED_IDENTITY_FIELDS)): + msg = ( + f"PatchAttrs no longer accepts {removed}; these are now spelled " + "as one data_source_id: network.fiber_array.location.acquisition." + ) + raise ValueError(msg) data.pop("dims", None) return data🤖 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 `@dascore/core/attrs.py` around lines 88 - 99, Update PatchAttrs.reject_coordinate_attributes to explicitly reject the removed identity keys network and station, raising a clear migration error when either is present before extra attributes are accepted. Preserve the existing coords validation and dims removal behavior, and follow Acquisition._reject_start_distance’s migration-message pattern.dascore/proc/inventory.py (1)
470-480: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
optical_distancesparameter.The only call site passes
distancesfor both parameters, so the two names describe one array. The split signature suggests they can differ, which invites a future caller to pass mismatched arrays into the"distance"branch.♻️ Proposed refactor
-def _get_coord_values(inventory, path, name, distances, optical_distances): +def _get_coord_values(inventory, path, name, distances): """Return the values of one requested coordinate, or None if undefined.""" if name == "distance": # Optical path distance is in meters, as every path length is. - return get_coord(data=optical_distances, units="m") + return get_coord(data=distances, units="m")- values = _get_coord_values(inventory, path, name, distances, distances) + values = _get_coord_values(inventory, path, name, distances)Also applies to: 524-524
🤖 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 `@dascore/proc/inventory.py` around lines 470 - 480, Remove the redundant optical_distances parameter from _get_coord_values and use distances directly in the "distance" branch. Update the function’s call site to pass only distances, preserving all other coordinate-resolution behavior.dascore/io/prodml/utils.py (1)
347-347: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider accepting
acquisition_idas a deprecated input alias.The writer now reads only
experiment_id. A patch that still carriesacquisition_idgets a freshuuid4()as itsAcquisitionId, so the recorded campaign identity is lost without any message. Issue#854recommended keepingacquisition_idas a deprecated alias with a warning.🤖 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 `@dascore/io/prodml/utils.py` at line 347, Update the acquisition ID handling around the `"AcquisitionId"` attribute so it first uses `experiment_id`, falls back to the deprecated `acquisition_id` alias when needed, and only generates a new UUID when neither is provided. Emit a deprecation warning whenever `acquisition_id` is used, while preserving the existing string conversion.tests/test_proc/test_proc_inventory.py (1)
646-657: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
andguard; callenrichdirectly.
with_far.networksis always a non-empty tuple here, sowith_far.networks and patch.enrich(...)always evaluates to the enrich result. The guard adds no coverage and hides the intent of the assertion.♻️ Proposed simplification
near = patch.enrich(inventory, attrs=False, coords=("coupling.medium",)) - both = with_far.networks and patch.enrich( - with_far, attrs=False, coords=("coupling.medium",) - ) + both = patch.enrich(with_far, attrs=False, coords=("coupling.medium",))🤖 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_proc/test_proc_inventory.py` around lines 646 - 657, Remove the redundant with_far.networks and guard in test_endpoint_belongs_to_its_own_run and call patch.enrich(with_far, attrs=False, coords=("coupling.medium",)) directly when assigning both, preserving the existing assertions.
🤖 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/spool.py`:
- Around line 80-102: Update _combine_inventories so a union with an inventory
attached to only one operand is rejected rather than returning that inventory
and enrichment arguments. Preserve the existing rejection for two differing
attachments, and only return inventory data when both operands carry the same
attachment.
- Around line 681-684: Update the spool-copy logic around __class__(self) so
refreshed spools preserve the source spool’s _inventory and _enrich_kwargs
attachment state, including when single-file update() rebuilds via from_file.
Copy both fields to the returned spool before returning it, while retaining the
existing enrich-based kwargs behavior where applicable.
In `@dascore/io/index/schema.py`:
- Around line 286-288: Remove the duplicated "source_path" and "source_format"
members from the set near "source_version" in the schema definition, leaving
only the new "source_version" entry while preserving the existing members.
In `@dascore/proc/inventory.py`:
- Around line 171-174: Update the data-state attribute lookup loop in
_get_system_attrs to call getattr with a None default, matching the existing
_get_system_attrs readers and allowing missing Acquisition fields to continue
through the on_missing path instead of raising AttributeError.
- Line 377: Update the zip call constructing spans to pass strict=True,
preserving the existing filtering and tuple construction while ensuring
intervals and values length mismatches raise instead of truncating.
- Around line 442-452: Update _get_geometry_coord to resolve the CRS axis index
defensively, treating an unknown coordinate label as a missing value instead of
allowing crs.axis_index(label) to raise InvalidInventoryError. Return None for
labels outside the CRS so the existing values-is-None handling can apply the
caller’s on_missing behavior.
In `@docs/recipes/real_time_proc.qmd`:
- Around line 112-113: Update the output-path construction around source_path
and output_path to extract only the source file’s basename, then append a
patch-specific suffix using patch_num so multiple patches cannot collide. Join
this sanitized patch filename with output_data_dir, ensuring absolute or nested
source paths never escape the output directory.
In `@tests/test_core/test_spool.py`:
- Around line 468-473: Require both selection examples to verify non-empty
results: in tests/test_core/test_spool.py lines 468-473, update
test_select_data_source_id to assert len(out) > 0 before iterating; in
docs/notes/spool_selection.qmd lines 31-32, add assert len(selected) > 0 before
the existing all(...) assertion.
In `@tests/test_io/test_common_io.py`:
- Around line 707-713: Update test_no_storage_provenance_attrs to include
source_path, source_format, and source_version in the prohibited PatchAttrs
field set, while preserving the existing rejection of path, file_format, and
file_version.
In `@tests/test_io/test_febus/test_febusbsl.py`:
- Around line 81-84: Update test_read_attrs_omit_storage_provenance to include
source_path, source_format, and source_version in the forbidden attribute-name
set, while preserving the existing assertions for path, file_format, and
file_version.
In `@tests/test_io/test_febus/test_febusg1.py`:
- Around line 178-182: Update test_read_attrs_omit_storage_provenance in
tests/test_io/test_febus/test_febusg1.py (lines 178-182) to assert absence of
source_path, source_format, and source_version alongside the legacy names;
update the corresponding SOR scan reader test in
tests/test_io/test_sr4731/test_sr4731.py (lines 183-185) with the same source_*
absence assertion.
In `@tests/test_io/test_xml_binary/test_xml_binary.py`:
- Around line 295-305: Update test_read_omits_provenance and
test_scan_omits_provenance to also reject source_path, source_format, and
source_version in the attribute-name assertions, preserving the existing checks
for the legacy provenance fields.
In `@tests/test_utils/test_patch_utils.py`:
- Line 872: Replace the "/tmp/real_file.h5" test value in the source_path
assignment with a neutral absolute non-memory path, preserving the existing test
behavior and the "memory://registry/patch" value.
---
Nitpick comments:
In `@dascore/core/attrs.py`:
- Around line 88-99: Update PatchAttrs.reject_coordinate_attributes to
explicitly reject the removed identity keys network and station, raising a clear
migration error when either is present before extra attributes are accepted.
Preserve the existing coords validation and dims removal behavior, and follow
Acquisition._reject_start_distance’s migration-message pattern.
In `@dascore/io/prodml/utils.py`:
- Line 347: Update the acquisition ID handling around the `"AcquisitionId"`
attribute so it first uses `experiment_id`, falls back to the deprecated
`acquisition_id` alias when needed, and only generates a new UUID when neither
is provided. Emit a deprecation warning whenever `acquisition_id` is used, while
preserving the existing string conversion.
In `@dascore/proc/inventory.py`:
- Around line 470-480: Remove the redundant optical_distances parameter from
_get_coord_values and use distances directly in the "distance" branch. Update
the function’s call site to pass only distances, preserving all other
coordinate-resolution behavior.
In `@tests/test_proc/test_proc_inventory.py`:
- Around line 646-657: Remove the redundant with_far.networks and guard in
test_endpoint_belongs_to_its_own_run and call patch.enrich(with_far,
attrs=False, coords=("coupling.medium",)) directly when assigning both,
preserving the existing assertions.
🪄 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: 219f551e-782a-44ba-b61a-cd59412ec4ae
📒 Files selected for processing (90)
benchmarks/test_spool_benchmarks.pydascore/config.pydascore/constants.pydascore/core/attrs.pydascore/core/inventory.pydascore/core/patch.pydascore/core/spool.pydascore/core/summary.pydascore/examples.pydascore/io/ap_sensing/utils.pydascore/io/dasdae/core.pydascore/io/dashdf5/core.pydascore/io/febus/a1utils.pydascore/io/febus/core.pydascore/io/febus/g1utils.pydascore/io/febus/t1utils.pydascore/io/gdr/core.pydascore/io/gdr/utils_das.pydascore/io/h5simple/core.pydascore/io/h5simple/utils.pydascore/io/index/backend.pydascore/io/index/catalog.pydascore/io/index/ingest.pydascore/io/index/planned.pydascore/io/index/schema.pydascore/io/neubrex/core.pydascore/io/neubrex/utils_das.pydascore/io/odh4/core.pydascore/io/optodas/core.pydascore/io/optodas/utils.pydascore/io/prodml/core.pydascore/io/prodml/utils.pydascore/io/segy/core.pydascore/io/segy/utils.pydascore/io/silixah5/core.pydascore/io/silixah5/utils.pydascore/io/sintela/core.pydascore/io/sintela/protobuf_utils.pydascore/io/sr4731/core.pydascore/io/sr4731/utils.pydascore/io/tdms/utils.pydascore/io/terra15/utils.pydascore/io/utils.pydascore/io/xml_binary/core.pydascore/io/xml_binary/utils.pydascore/proc/__init__.pydascore/proc/inventory.pydascore/utils/chunk_plan.pydascore/utils/misc.pydascore/utils/patch.pydascore/utils/paths.pydocs/changelog.qmddocs/contributing/new_format.qmddocs/notes/spool_chunking.qmddocs/notes/spool_index.qmddocs/notes/spool_selection.qmddocs/recipes/real_time_proc.qmddocs/tutorial/file_io.qmddocs/tutorial/patch.qmddocs/tutorial/spool.qmdtests/test_core/test_attrs.pytests/test_core/test_directory_spool.pytests/test_core/test_file_spool.pytests/test_core/test_inventory.pytests/test_core/test_patch_chunk.pytests/test_core/test_spool.pytests/test_io/test_common_io.pytests/test_io/test_dasdae/test_dasdae.pytests/test_io/test_febus/test_febusbsl.pytests/test_io/test_febus/test_febusg1.pytests/test_io/test_index/test_catalog.pytests/test_io/test_index/test_db_dirspool.pytests/test_io/test_index/test_heterogeneity_stress.pytests/test_io/test_index/test_hive_attrs.pytests/test_io/test_index/test_index_contract.pytests/test_io/test_index/test_index_edge_cases.pytests/test_io/test_index/test_plan.pytests/test_io/test_index/test_planned.pytests/test_io/test_index/test_union.pytests/test_io/test_indexer.pytests/test_io/test_io_core.pytests/test_io/test_pickle/test_pickle.pytests/test_io/test_prodml/test_prodml_write.pytests/test_io/test_sr4731/test_sr4731.pytests/test_io/test_xml_binary/test_xml_binary.pytests/test_proc/test_proc_inventory.pytests/test_utils/test_attrs_utils.pytests/test_utils/test_chunk.pytests/test_utils/test_config.pytests/test_utils/test_patch_utils.py
💤 Files with no reviewable changes (6)
- dascore/io/gdr/core.py
- dascore/io/odh4/core.py
- dascore/io/sintela/core.py
- dascore/io/neubrex/core.py
- dascore/io/silixah5/core.py
- dascore/io/sr4731/core.py
Comment on lines
+80
to
+102
| def _combine_inventories(first, second) -> tuple: | ||
| """ | ||
| Return the (inventory, enrich kwargs) a union of two spools carries. | ||
|
|
||
| An inventory attached to only one operand still describes the patches | ||
| it came with, so it carries over; two different attachments have no | ||
| single answer and say so. | ||
| """ | ||
| pairs = [ | ||
| (getattr(x, "_inventory", None), getattr(x, "_enrich_kwargs", None)) | ||
| for x in (first, second) | ||
| ] | ||
| attached = [x for x in pairs if x[0] is not None] | ||
| if not attached: | ||
| return None, None | ||
| if len(attached) == 2 and attached[0] != attached[1]: | ||
| msg = ( | ||
| "The spools carry different inventories (or different enrich " | ||
| "arguments), which have no combined meaning. Attach one " | ||
| "inventory to the combined spool instead." | ||
| ) | ||
| raise InvalidSpoolError(msg) | ||
| return attached[0] |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not retain an inventory attached to only one union operand.
The returned spool enriches every extracted patch. A single input inventory only describes patches from that input. It can enrich patches from the other spool with unrelated metadata.
Reject this union state, or require callers to attach an inventory after the union.
Proposed fix
attached = [x for x in pairs if x[0] is not None]
if not attached:
return None, None
+ if len(attached) == 1:
+ raise InvalidSpoolError(
+ "Only one spool has an attached inventory. "
+ "Attach an inventory to the combined spool instead."
+ )
if len(attached) == 2 and attached[0] != attached[1]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _combine_inventories(first, second) -> tuple: | |
| """ | |
| Return the (inventory, enrich kwargs) a union of two spools carries. | |
| An inventory attached to only one operand still describes the patches | |
| it came with, so it carries over; two different attachments have no | |
| single answer and say so. | |
| """ | |
| pairs = [ | |
| (getattr(x, "_inventory", None), getattr(x, "_enrich_kwargs", None)) | |
| for x in (first, second) | |
| ] | |
| attached = [x for x in pairs if x[0] is not None] | |
| if not attached: | |
| return None, None | |
| if len(attached) == 2 and attached[0] != attached[1]: | |
| msg = ( | |
| "The spools carry different inventories (or different enrich " | |
| "arguments), which have no combined meaning. Attach one " | |
| "inventory to the combined spool instead." | |
| ) | |
| raise InvalidSpoolError(msg) | |
| return attached[0] | |
| def _combine_inventories(first, second) -> tuple: | |
| """ | |
| Return the (inventory, enrich kwargs) a union of two spools carries. | |
| An inventory attached to only one operand still describes the patches | |
| it came with, so it carries over; two different attachments have no | |
| single answer and say so. | |
| """ | |
| pairs = [ | |
| (getattr(x, "_inventory", None), getattr(x, "_enrich_kwargs", None)) | |
| for x in (first, second) | |
| ] | |
| attached = [x for x in pairs if x[0] is not None] | |
| if not attached: | |
| return None, None | |
| if len(attached) == 1: | |
| raise InvalidSpoolError( | |
| "Only one spool has an attached inventory. " | |
| "Attach an inventory to the combined spool instead." | |
| ) | |
| if len(attached) == 2 and attached[0] != attached[1]: | |
| msg = ( | |
| "The spools carry different inventories (or different enrich " | |
| "arguments), which have no combined meaning. Attach one " | |
| "inventory to the combined spool instead." | |
| ) | |
| raise InvalidSpoolError(msg) | |
| return attached[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 `@dascore/core/spool.py` around lines 80 - 102, Update _combine_inventories so
a union with an inventory attached to only one operand is rejected rather than
returning that inventory and enrichment arguments. Preserve the existing
rejection for two differing attachments, and only return inventory data when
both operands carry the same attachment.
Comment on lines
+681
to
+684
| new = self.__class__(self) | ||
| new._inventory = inventory | ||
| new._enrich_kwargs = dict(kwargs) if enrich else None | ||
| return new |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve attachment state when a file-backed spool updates.
attach_inventory stores state on the returned spool. A single-file update() returns a new spool from from_file, so it drops _inventory and _enrich_kwargs. The same spool yields different patch metadata after update().
Copy both fields to the refreshed spool before returning it.
Proposed fix
- return self.from_file(
+ updated = self.from_file(
self._file_path, self._file_format, self._file_version
)
+ updated._inventory = self._inventory
+ updated._enrich_kwargs = self._enrich_kwargs
+ return updated🤖 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 `@dascore/core/spool.py` around lines 681 - 684, Update the spool-copy logic
around __class__(self) so refreshed spools preserve the source spool’s
_inventory and _enrich_kwargs attachment state, including when single-file
update() rebuilds via from_file. Copy both fields to the returned spool before
returning it, while retaining the existing enrich-based kwargs behavior where
applicable.
Comment on lines
+286
to
+288
| "source_path", | ||
| "source_format", | ||
| "source_version", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicated set members.
"source_path" and "source_format" are already members at Lines 259-260. Only "source_version" is new. Ruff flags the repeats (B033), so lint fails on this hunk.
🧹 Proposed fix
- # flat-relation (spool-facing) names
- "source_path",
- "source_format",
+ # flat-relation (spool-facing) names; source_path and source_format
+ # keep their storage-table spelling in the flat relation, so the
+ # entries above already reserve them.
"source_version",🧰 Tools
🪛 Ruff (0.16.1)
[warning] 286-286: Sets should not contain duplicate item "source_path"
Remove duplicate item
(B033)
[warning] 287-287: Sets should not contain duplicate item "source_format"
Remove duplicate item
(B033)
🤖 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 `@dascore/io/index/schema.py` around lines 286 - 288, Remove the duplicated
"source_path" and "source_format" members from the set near "source_version" in
the schema definition, leaving only the new "source_version" entry while
preserving the existing members.
Source: Linters/SAST tools
| """ | ||
| fill = {"boolean": False, "numeric": np.nan}.get(kind, None) | ||
| out = np.full(len(distances), fill, dtype=object) | ||
| spans = [(lo, hi, value) for (lo, hi), value in zip(intervals, values) if lo < hi] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add strict=True to the zip call.
Ruff flags this call (B905). intervals and values are built from the same item list, so strict=True is free and makes any future divergence fail loudly instead of truncating a track silently.
🧹 Proposed fix
- spans = [(lo, hi, value) for (lo, hi), value in zip(intervals, values) if lo < hi]
+ spans = [
+ (lo, hi, value)
+ for (lo, hi), value in zip(intervals, values, strict=True)
+ if lo < hi
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| spans = [(lo, hi, value) for (lo, hi), value in zip(intervals, values) if lo < hi] | |
| spans = [ | |
| (lo, hi, value) | |
| for (lo, hi), value in zip(intervals, values, strict=True) | |
| if lo < hi | |
| ] |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 377-377: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 `@dascore/proc/inventory.py` at line 377, Update the zip call constructing
spans to pass strict=True, preserving the existing filtering and tuple
construction while ensuring intervals and values length mismatches raise instead
of truncating.
Source: Linters/SAST tools
Comment on lines
+81
to
+84
| def test_read_attrs_omit_storage_provenance(self, bsl_patch): | ||
| """Where the bytes live belongs to the spool, not to patch attrs.""" | ||
| names = set(dict(bsl_patch.attrs)) | ||
| assert not names & {"path", "file_format", "file_version"} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the renamed storage provenance keys.
The test passes if the reader adds source_path, source_format, or source_version to patch attrs. This violates the new spool-owned provenance contract. Assert that these keys are absent too.
Proposed test update
- assert not names & {"path", "file_format", "file_version"}
+ assert not names & {
+ "path",
+ "file_format",
+ "file_version",
+ "source_path",
+ "source_format",
+ "source_version",
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_read_attrs_omit_storage_provenance(self, bsl_patch): | |
| """Where the bytes live belongs to the spool, not to patch attrs.""" | |
| names = set(dict(bsl_patch.attrs)) | |
| assert not names & {"path", "file_format", "file_version"} | |
| def test_read_attrs_omit_storage_provenance(self, bsl_patch): | |
| """Where the bytes live belongs to the spool, not to patch attrs.""" | |
| names = set(dict(bsl_patch.attrs)) | |
| assert not names & { | |
| "path", | |
| "file_format", | |
| "file_version", | |
| "source_path", | |
| "source_format", | |
| "source_version", | |
| } |
🤖 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_io/test_febus/test_febusbsl.py` around lines 81 - 84, Update
test_read_attrs_omit_storage_provenance to include source_path, source_format,
and source_version in the forbidden attribute-name set, while preserving the
existing assertions for path, file_format, and file_version.
Comment on lines
+178
to
+182
| def test_read_attrs_omit_storage_provenance(self, mtx_h5_path): | ||
| """Where the bytes live belongs to the spool, not to patch attrs.""" | ||
| patch = dc.read(mtx_h5_path)[0] | ||
| assert patch.attrs.path == str(mtx_h5_path) | ||
| assert patch.attrs.file_format == FebusMTXH5V1.name | ||
| assert patch.attrs.file_version == FebusMTXH5V1.version | ||
| names = set(dict(patch.attrs)) | ||
| assert not names & {"path", "file_format", "file_version"} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Cover the renamed provenance contract in both reader tests.
Both tests reject only the legacy names. A regression that leaks source_path, source_format, or source_version into PatchAttrs would pass.
tests/test_io/test_febus/test_febusg1.py#L178-L182: include all threesource_*names in the absence assertion.tests/test_io/test_sr4731/test_sr4731.py#L183-L185: add the same absence assertion for the SOR scan attributes.
📍 Affects 2 files
tests/test_io/test_febus/test_febusg1.py#L178-L182(this comment)tests/test_io/test_sr4731/test_sr4731.py#L183-L185
🤖 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_io/test_febus/test_febusg1.py` around lines 178 - 182, Update
test_read_attrs_omit_storage_provenance in
tests/test_io/test_febus/test_febusg1.py (lines 178-182) to assert absence of
source_path, source_format, and source_version alongside the legacy names;
update the corresponding SOR scan reader test in
tests/test_io/test_sr4731/test_sr4731.py (lines 183-185) with the same source_*
absence assertion.
Comment on lines
+314
to
+318
| def test_filter_tag_exact(self, diverse_df, diverse_indexer): | ||
| """Ensure contents can be filtered on an attr.""" | ||
| exact_name = diverse_df["station"].unique()[0] | ||
| new_df = diverse_indexer(station=exact_name) | ||
| assert (new_df["station"] == exact_name).all() | ||
| exact_name = diverse_df["tag"].unique()[0] | ||
| new_df = diverse_indexer(tag=exact_name) | ||
| assert (new_df["tag"] == exact_name).all() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a non-empty tag in the exact-match test.
Line 316 can select "", which the test identifies as non-queryable. The filter can then return no rows and all() can still pass. Select a non-empty tag and assert that the result contains at least one row.
Proposed test fix
- exact_name = diverse_df["tag"].unique()[0]
+ tags = [x for x in diverse_df["tag"].unique() if x]
+ exact_name = tags[0]
new_df = diverse_indexer(tag=exact_name)
+ assert len(new_df)
assert (new_df["tag"] == exact_name).all()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_filter_tag_exact(self, diverse_df, diverse_indexer): | |
| """Ensure contents can be filtered on an attr.""" | |
| exact_name = diverse_df["station"].unique()[0] | |
| new_df = diverse_indexer(station=exact_name) | |
| assert (new_df["station"] == exact_name).all() | |
| exact_name = diverse_df["tag"].unique()[0] | |
| new_df = diverse_indexer(tag=exact_name) | |
| assert (new_df["tag"] == exact_name).all() | |
| def test_filter_tag_exact(self, diverse_df, diverse_indexer): | |
| """Ensure contents can be filtered on an attr.""" | |
| tags = [x for x in diverse_df["tag"].unique() if x] | |
| exact_name = tags[0] | |
| new_df = diverse_indexer(tag=exact_name) | |
| assert len(new_df) | |
| assert (new_df["tag"] == exact_name).all() |
Comment on lines
+295
to
+305
| def test_read_omits_provenance(self, binary_xml_directory): | ||
| """A read patch carries no path, format, or version attr.""" | ||
| patch = dc.read(binary_xml_directory)[0] | ||
| names = set(dict(patch.attrs)) | ||
| assert not names & {"path", "file_format", "file_version"} | ||
|
|
||
| def test_scan_omits_provenance(self, binary_xml_directory): | ||
| """Neither does a scanned summary's attrs.""" | ||
| summary = dc.scan(binary_xml_directory)[0] | ||
| names = set(dict(summary.attrs)) | ||
| assert not names & {"path", "file_format", "file_version"} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Check the renamed provenance fields too.
The test rejects path, file_format, and file_version, but it does not reject source_path, source_format, or source_version. A regression that leaks the renamed fields into patch or summary attributes would pass this test despite violating the test description.
Proposed test fix
- assert not names & {"path", "file_format", "file_version"}
+ assert not names & {
+ "path",
+ "file_format",
+ "file_version",
+ "source_path",
+ "source_format",
+ "source_version",
+ }Apply the same assertion to both tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_read_omits_provenance(self, binary_xml_directory): | |
| """A read patch carries no path, format, or version attr.""" | |
| patch = dc.read(binary_xml_directory)[0] | |
| names = set(dict(patch.attrs)) | |
| assert not names & {"path", "file_format", "file_version"} | |
| def test_scan_omits_provenance(self, binary_xml_directory): | |
| """Neither does a scanned summary's attrs.""" | |
| summary = dc.scan(binary_xml_directory)[0] | |
| names = set(dict(summary.attrs)) | |
| assert not names & {"path", "file_format", "file_version"} | |
| def test_read_omits_provenance(self, binary_xml_directory): | |
| """A read patch carries no path, format, or version attr.""" | |
| patch = dc.read(binary_xml_directory)[0] | |
| names = set(dict(patch.attrs)) | |
| assert not names & { | |
| "path", | |
| "file_format", | |
| "file_version", | |
| "source_path", | |
| "source_format", | |
| "source_version", | |
| } | |
| def test_scan_omits_provenance(self, binary_xml_directory): | |
| """Neither does a scanned summary's attrs.""" | |
| summary = dc.scan(binary_xml_directory)[0] | |
| names = set(dict(summary.attrs)) | |
| assert not names & { | |
| "path", | |
| "file_format", | |
| "file_version", | |
| "source_path", | |
| "source_format", | |
| "source_version", | |
| } |
🤖 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_io/test_xml_binary/test_xml_binary.py` around lines 295 - 305,
Update test_read_omits_provenance and test_scan_omits_provenance to also reject
source_path, source_format, and source_version in the attribute-name assertions,
preserving the existing checks for the legacy provenance fields.
| """Mixed real and memory paths consistently use metadata names.""" | ||
| df = random_spool.get_contents().iloc[:2].copy() | ||
| df["path"] = ["/tmp/real_file.h5", "memory://registry/patch"] | ||
| df["source_path"] = ["/tmp/real_file.h5", "memory://registry/patch"] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid the /tmp literal so the S108 lint check passes.
Ruff reports S108 for "/tmp/real_file.h5". The test never opens the path; it only needs a non-memory path value. Use a neutral absolute path.
🔧 Proposed change
- df["source_path"] = ["/tmp/real_file.h5", "memory://registry/patch"]
+ df["source_path"] = ["/data/real_file.h5", "memory://registry/patch"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| df["source_path"] = ["/tmp/real_file.h5", "memory://registry/patch"] | |
| df["source_path"] = ["/data/real_file.h5", "memory://registry/patch"] |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 872-872: Probable insecure usage of temporary file or directory: "/tmp/real_file.h5"
(S108)
🤖 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_utils/test_patch_utils.py` at line 872, Replace the
"/tmp/real_file.h5" test value in the source_path assignment with a neutral
absolute non-memory path, preserving the existing test behavior and the
"memory://registry/patch" value.
Source: Linters/SAST tools
A review pass that ran real workflows against dev and against this branch, plus the first CI run. Upgrading: - Default patch names now contain dots, because data_source_id does, and get_patch_names stripped everything after the first dot when reading a name back off a path. Writing patches under their own names and re-indexing collapsed every patch of one source onto one truncated, colliding name. It strips the extension only. - An archive partitioned by network=/station= hive keys stopped being partitioned when those left the default groupby_attrs, so chunking merged patches from different places -- silently discarding one of them under conflict="drop". They stay in the default: a name missing from a spool is ignored, grouping too finely only leaves patches unmerged, and grouping too coarsely loses data. - A patch written before the units were fixed still carries the companion attr naming its own, so the ProdML writer honors it instead of stamping seconds on a value in nanoseconds. - The index columns kept their names while their vocabulary and units changed, which no version check could catch. The index version is bumped, so a stale index is rebuilt rather than mixing the two. - A spool carrying an inventory did not equal its own pickle: deep_equality_check walks a model's fields, where an unset NaT time never equals itself. Inventories compare with == in __eq__ instead. CI: - The lint hook wants the future import wherever annotations appear. - The docs link validator resolves callables, not module constants. - NetCDF hands back the attrs stored in the file, like DASDAE, so the file rather than the reader chooses the names; the vocabulary test exempts both and lists MiniSEED's own reader-chosen names.
Two review passes looking for helpers that do not earn their keep and for rules stated more than once. The half-open coverage rule with the run end included was written twice, in coordinates_at for geometry and in _fill_from_intervals for the tracks. It is one rule, so it is one function, interval_masks, which the model owns and enrichment uses. The "paired, finite, strictly increasing" control-point check was written three times across DistanceMap and Geometry; it is now one check with the object's name passed in. enrich carried its own _values_equal which returned False for any array attr, because bool(array == array) raises and it caught that as a disagreement. The models' own comparison already handles arrays, nulls, mappings and sequences, so it is promoted to public and used instead. The dotted-name owner lookup is one helper rather than two spellings, and the on_missing values derive from the signature's own Literal instead of being written twice. convert_attr_units swaps a parameter no reader used for one two readers need: a from_units default for headers whose units live in the key name, which is what silixa's private converter was doing. Gone entirely: a segy wrapper whose body no longer read the header it named, an sr4731 extras chain whose last caller this branch removed, a prodml wrapper restating its own loop, a febus zero-argument function returning a constant, and a bool() cast around one numpy call.
Splatting a plain dict widens the Literal-typed data_type and data_category to str; from_dict validates the same mapping without claiming to. The prodml measure is checked for absence before float(), which cannot take None.
|
✅ Documentation built: |
The reader emits the interrogator's facts under their dotted names; the test still asked for the vendor spellings, which only CI could see because the protobuf reader needs an optional dependency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 1070: Update the zip call in the geometry/mask iteration to use
strict=True, enforcing that self.geometry and masks contain exactly one
corresponding entry per interval as guaranteed by interval_masks.
🪄 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: 87bc1b7f-cf63-46bf-9c00-db42a1e228fe
📒 Files selected for processing (24)
dascore/config.pydascore/constants.pydascore/core/inventory.pydascore/core/spool.pydascore/io/febus/t1utils.pydascore/io/index/schema.pydascore/io/prodml/utils.pydascore/io/segy/core.pydascore/io/segy/utils.pydascore/io/silixah5/utils.pydascore/io/sr4731/utils.pydascore/io/utils.pydascore/proc/inventory.pydascore/utils/models.pydascore/utils/patch.pydocs/changelog.qmddocs/contributing/new_format.qmdtests/test_core/test_inventory.pytests/test_io/test_common_io.pytests/test_io/test_io_core.pytests/test_io/test_sintela/test_protobuf.pytests/test_proc/test_proc_inventory.pytests/test_utils/test_config.pytests/test_utils/test_patch_utils.py
💤 Files with no reviewable changes (1)
- dascore/io/segy/utils.py
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/changelog.qmd
- dascore/io/index/schema.py
- dascore/io/utils.py
- dascore/io/febus/t1utils.py
- tests/test_utils/test_patch_utils.py
- docs/contributing/new_format.qmd
- tests/test_io/test_io_core.py
- dascore/constants.py
- dascore/utils/patch.py
- dascore/io/prodml/utils.py
- dascore/core/spool.py
- dascore/io/silixah5/utils.py
- tests/test_core/test_inventory.py
| coords = np.asarray(segment.coordinates, dtype=float) | ||
| out[at_outer] = coords[-1] | ||
| masks = interval_masks(dist, [x.interval for x in self.geometry]) | ||
| for segment, mask in zip(self.geometry, masks): |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target context ---'
sed -n '1025,1090p' dascore/core/inventory.py
printf '%s\n' '--- interval_masks definitions and uses ---'
rg -n -A35 -B8 'def interval_masks|interval_masks\(' dascore
printf '%s\n' '--- project Python requirements and lint configuration ---'
rg -n -A8 -B4 'requires-python|target-version|B905|ruff' pyproject.toml setup.cfg tox.ini .ruff.toml 2>/dev/null || true
printf '%s\n' '--- zip call count in target file ---'
rg -n 'zip\(' dascore/core/inventory.pyRepository: DASDAE/dascore
Length of output: 16387
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- complete Ruff selection ---'
sed -n '173,205p' pyproject.toml
printf '%s\n' '--- standalone interval cardinality and strict-zip probe ---'
python3 - <<'PY'
from itertools import zip_longest
import numpy as np
def interval_masks(values, intervals):
values = np.asarray(values, dtype=float)
spans = [(lo, hi) for lo, hi in intervals]
claimed = np.zeros(len(values), dtype=bool)
for lo, hi in spans:
if lo < hi:
claimed |= (values >= lo) & (values < hi)
out = []
for lo, hi in spans:
if lo >= hi:
out.append(np.zeros(len(values), dtype=bool))
continue
mask = (values >= lo) & (values < hi)
out.append(mask | ((values == hi) & ~claimed))
return out
cases = [
([], []),
([0, 1, 2], [(0, 1), (1, 2)]),
([0, 1, 2], [(0, 0), (1, 2), (2, 2)]),
]
for values, intervals in cases:
masks = interval_masks(values, intervals)
assert len(masks) == len(intervals), (intervals, len(masks))
list(zip(intervals, masks, strict=True))
print("interval_masks cardinality matches intervals for empty, overlapping, and point-marker cases")
try:
list(zip([1, 2], ["only"], strict=True))
except ValueError as exc:
print(f"strict zip mismatch raises ValueError: {exc}")
else:
raise AssertionError("strict zip did not reject unequal lengths")
PY
printf '%s\n' '--- Ruff B905 check, if available ---'
if command -v ruff >/dev/null 2>&1; then
ruff check --select B905 dascore/core/inventory.py
else
echo 'ruff is not installed in the sandbox'
fiRepository: DASDAE/dascore
Length of output: 804
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- standalone cardinality probe without third-party dependencies ---'
python3 - <<'PY'
def interval_masks(values, intervals):
spans = [(lo, hi) for lo, hi in intervals]
claimed = [False] * len(values)
for lo, hi in spans:
if lo < hi:
for i, value in enumerate(values):
claimed[i] |= lo <= value < hi
out = []
for lo, hi in spans:
if lo >= hi:
out.append([False] * len(values))
continue
out.append([
(lo <= value < hi) or (value == hi and not claimed[i])
for i, value in enumerate(values)
])
return out
cases = [
([], []),
([0, 1, 2], [(0, 1), (1, 2)]),
([0, 1, 2], [(0, 0), (1, 2), (2, 2)]),
]
for values, intervals in cases:
masks = interval_masks(values, intervals)
assert len(masks) == len(intervals), (intervals, len(masks))
list(zip(intervals, masks, strict=True))
print("interval_masks cardinality matches intervals for all probe cases")
try:
list(zip([1, 2], ["only"], strict=True))
except ValueError as exc:
print(f"strict zip mismatch raises ValueError: {exc}")
else:
raise AssertionError("strict zip did not reject unequal lengths")
PY
printf '%s\n' '--- Ruff availability and B905 references ---'
if command -v ruff >/dev/null 2>&1; then
ruff --version
ruff check --select B905 dascore/core/inventory.py || true
else
echo 'ruff is not installed in the sandbox'
fi
rg -n 'B905|bugbear|select.*B' pyproject.toml setup.cfg tox.ini .github 2>/dev/null || trueRepository: DASDAE/dascore
Length of output: 340
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- standalone interval cardinality probe ---'
python3 - <<'PY'
import sys
def interval_masks(values, intervals):
spans = [(lo, hi) for lo, hi in intervals]
claimed = [False] * len(values)
for lo, hi in spans:
if lo < hi:
for i, value in enumerate(values):
claimed[i] |= lo <= value < hi
out = []
for lo, hi in spans:
if lo >= hi:
out.append([False] * len(values))
continue
out.append([
(lo <= value < hi) or (value == hi and not claimed[i])
for i, value in enumerate(values)
])
return out
cases = [
([], []),
([0, 1, 2], [(0, 1), (1, 2)]),
([0, 1, 2], [(0, 0), (1, 2), (2, 2)]),
]
for values, intervals in cases:
masks = interval_masks(values, intervals)
assert len(masks) == len(intervals), (intervals, len(masks))
print("interval_masks cardinality matches intervals for all probe cases")
print(f"runtime={sys.version.split()[0]}")
PY
printf '%s\n' '--- Ruff availability and B905 references ---'
if command -v ruff >/dev/null 2>&1; then
ruff --version
ruff check --select B905 dascore/core/inventory.py
printf 'ruff_exit=%s\n' "$?"
else
echo 'ruff is not installed in the sandbox'
fi
rg -n 'B905|bugbear|select.*B' pyproject.toml setup.cfg tox.ini .github 2>/dev/null || trueRepository: DASDAE/dascore
Length of output: 1182
Declare the required one-to-one iteration.
Set strict=True on this zip call. interval_masks returns exactly one mask for each geometry interval.
Proposed fix
- for segment, mask in zip(self.geometry, masks):
+ for segment, mask in zip(self.geometry, masks, strict=True):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for segment, mask in zip(self.geometry, masks): | |
| for segment, mask in zip(self.geometry, masks, strict=True): |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 1070-1070: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 `@dascore/core/inventory.py` at line 1070, Update the zip call in the
geometry/mask iteration to use strict=True, enforcing that self.geometry and
masks contain exactly one corresponding entry per interval as guaranteed by
interval_masks.
Source: Linters/SAST tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #855 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 176 177 +1
Lines 19372 19749 +377
==========================================
+ Hits 19372 19749 +377
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:
|
Febus' own reference library declares PulseWidth with unit "m" (febus_optics_lib 1.4.2, plugins/plugins_das_febus.py), which settles it: the value is a length, not a time, and keeps terra15's pulse_length name. The legacy group attrs say they are legacy, in the comment and in the config field's own description. And the reason sample_rate and spatial_interval sit out of blanket enrichment is stated as what it is: nothing should be redundant between a patch's coordinates and its attrs, and those two are what the time and distance coordinates already say.
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.
Description
Phase 2 of the DASDAE inventory work, on top of the model layer merged in #843: the patch surface. It gives a patch a single identity the inventory can resolve, makes the readers speak the inventory's own vocabulary, and adds the operation that joins the two.
Patch attrs are reorganized around one identity field.
data_source_idholds the whole inventory identity (network.fiber_array.location.acquisition), replacingnetwork,station,instrument_id, andacquisition_id. Component queries are wildcard matches on the composite (spool.select(data_source_id="XX.*")).stationis not reused because DAS MiniSEED already spends it on a per-sampling-point identity.Readers speak one vocabulary, shared with the inventory.
constants.INVENTORY_ATTRSlists the observing-system facts a reader may emit; each is a field of the inventory's acquisition, or of its interrogator when dotted (interrogator.serial_number). Those attrs carry fixed units, so readers convert at the parse boundary through the newio.utils.convert_attr_unitsand the*_unitscompanion attrs are gone. A conformance test holds every shipped reader to the vocabulary; vendor-specific attrs are untouched and listed explicitly.Storage provenance is spelled
source_*everywhere — theget_contents()and index columnspath/file_format/file_versionbecomesource_path/source_format/source_version, matchingPatchSummary— and no reader puts them in patch attrs: they say where the bytes live, and a patch merged from three files has no single answer.Patch.enrichandSpool.attach_inventorycopy inventory metadata onto a patch: the patch'sdata_source_idand time resolve one acquisition and optical path, the acquisition's channel map places each channel on the path, and the path's tracks project onto it.attrsandcoordseach takeTrue, a tuple of names, orFalse;conflictssettles disagreements using chunking's vocabulary;on_missinggoverns names the inventory does not define.One model rule changed while implementing it: an acquisition now places its channels one way, the
distance_map. The affinestart_distanceform mapped channel numbers, which no reader produces (36 of the 38 example files carry meters, none carry channels), and a single control point does the same job while extrapolating. The map may now state its points in both input coordinates, and the patch's own coordinate decides which is read.All breaking changes are described in
docs/changelog.qmd.Reviewed before opening: one Codex pass and three adversarial review passes on the attrs/vocabulary work, plus one Codex and three adversarial passes on the channel-resolution change. Those found (and this PR fixes) a Febus pulse width converted as nanoseconds when the header states meters, a second Silixa code path that skipped its conversion, an attr name that broke every trimmed read of a spool, a hive path that indexed an unusable
data_source_idand failed only at load, and a dual-axis map that could silently be read on the wrong coordinate.Checklist
I have (if applicable):
Closes #854.
Summary by CodeRabbit
New Features
data_source_idmetadata, standardized interrogator attributes, and canonical unit conversion across supported formats.Improvements
source_path,source_format, andsource_version.Documentation