🐛 fix(interop-xarray): reject quantify() unit specs for unknown names - #794
Merged
Merged
Conversation
`attach_units` iterated the object's variables/coords with `units.get(name)`, so a key that matched nothing -- e.g. a typo in a `**unit_kwargs` name like `ds.unxt.quantify(temperatrue="K")` -- was silently dropped, leaving the data completely unquantified with no error or warning. Validate the supplied names against the object's variables/coordinates (plus the `None` key for a DataArray's own data) and raise a `ValueError` naming the unmatched keys, for both the DataArray and Dataset dispatches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
🧩 unxts-interop-xarray
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the unxts.interop.xarray unit-attachment path so that quantify()/attach_units() no longer silently ignore unit specifications for unknown variable/coordinate names (e.g., typos), and instead raise a ValueError identifying the bad keys.
Changes:
- Add
_reject_unknown_unit_names()and call it from bothDataArrayandDatasetattach_unitsdispatches to validate supplied unit keys. - Add regression tests ensuring typos in unit keyword names raise for both
DatasetandDataArray, while valid names still quantify correctly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/unxts.interop.xarray/src/unxts/interop/xarray/_src/conversion.py | Validate unit-name keys before attaching units, raising on unknown names. |
| packages/unxts.interop.xarray/tests/test_unknown_unit_keys.py | Regression tests covering unknown unit-name keys for both Dataset and DataArray. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
+ Coverage 91.85% 94.75% +2.90%
==========================================
Files 83 48 -35
Lines 3734 2748 -986
Branches 310 199 -111
==========================================
- Hits 3430 2604 -826
+ Misses 228 102 -126
+ Partials 76 42 -34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Address PR GalacticDynamics#794 review: the `None` (DataArray own-data) key was filtered out of the "Valid names" list, so a DataArray with no coords produced the confusing `Valid names: .`. Render `None` as `None (the data)` and fall back to `(none)` when the valid set is otherwise empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Early-return on the no-unknowns case instead of nesting the whole error-building block under ``if unknown:``, and split the valid-names generator onto its own line so the ``sorted(...) or "(none)"`` fallback reads flat rather than through three levels of parens. Behaviour is unchanged; net 4 lines shorter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR GalacticDynamics#794 review: - Sort the unknown-key list in the ValueError. `units` insertion order follows the accessor's `set(...)` iteration, which is not stable across processes, so the message was non-deterministic with multiple bad keys. - Assert the coordinate unit in test_dataarray_typo_coord_key_raises. Switched to a non-dimension coordinate because a dimension coordinate is pandas-indexed by xarray and drops its unit (a separate, deferred limitation); the non-dim coord verifies a valid coord name actually quantifies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
attach_unitsiterated the object's variables/coords doingunits.get(name), so any key that matched nothing — most easily a typo in a**unit_kwargsname — was silently dropped:Now both the DataArray and Dataset
attach_unitsdispatches validate the supplied names against the object's variables/coordinates (plus theNonekey for a DataArray's own data) and raise aValueErrornaming the unmatched keys.Verification (TDD)
New
test_unknown_unit_keys.py: a typo'd key raises for bothDatasetandDataArray(naming the bad key), while the correct names still quantify.unxts.interop.xarraytests: 15 passed.Notes
unitsattr after quantify" finding is already fixed (verified — attrs come back{}; handled by 🐛 fix(interop): drop consumed xarray units attr #748).attach_units, so a naive warning is noisy and breaks the roundtrip contract.🤖 Generated with Claude Code