Fix unit extraction in structured_to_unstructured for StructuredQuantity#19106
Fix unit extraction in structured_to_unstructured for StructuredQuantity#19106ReemHamraz wants to merge 3 commits into
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
| # For the other tests of ``structured_to_unstructured``, see | ||
| # ``test_quantity_non_ufuncs.TestRecFunctions.test_unstructured_to_structured`` | ||
|
|
||
| def test_structured_to_unstructured_success(self): |
There was a problem hiding this comment.
a crucial property of a test that accompanies a change is that, in addition to passing with the test, it should also fail without the change. This way we know that the test properly captures the change in behavior that we intend it to check. This test doesn't fail on main, so, maybe it's still valuable in itself (I haven't attempted to evaluate this yet), but it is demonstrably insufficient to check your change.
There was a problem hiding this comment.
I think it makes more sense in that other module
|
thanks @ReemHamraz ! your test is now failing as expected on main, so it qualifies as a proper regression test ! |
|
good news: docs failures are unrelated, and seen across all PRs |
ReemHamraz
force-pushed
the
fix-structured-unit-extraction
branch
from
674767c to
39aa191
Compare
|
pre-commit.ci autofix |
|
Errr I don't see any code changes, just a change log entry. |
|
Huho, this is probably the result of a failed rebase+squash. Luckily I remember having checked-out this branch locally last time I reviewed it so I should be able to re-play the intended changes authored by @ReemHamraz |
neutrinoceros
force-pushed
the
fix-structured-unit-extraction
branch
from
7f053e3 to
0606db8
Compare
Co-authored-by: Clément Robert <[email protected]>
| with pytest.raises(ValueError, match="The length of the last dimension"): | ||
| rfn.unstructured_to_structured(self.q_pv, dtype=self.q_pv.dtype) | ||
|
|
||
| # For the other tests of ``structured_to_unstructured``, see |
There was a problem hiding this comment.
just seeing this comment now... should your test be moved there ? (I'll try to answer my own question right now)
3 participants
Fix unit extraction in
structured_to_unstructuredforStructuredQuantityDescription
This PR addresses a bug where
numpy.lib.recfunctions.structured_to_unstructuredfailed when applied to aStructuredQuantity, even if all fields within the quantity possessed compatible units.The Problem
The function was failing during the unit extraction phase. Because a
StructuredQuantityhas aStructuredUnitinstead of a standardUnitobject, the existing helper logic was attempting to access attributes or iterators that did not exist for that object type, leading to either anAttributeErroror aStopIteration.The Fix
I modified the unit extraction logic in
astropy/units/quantity_helper/function_helpers.pyto safely handleStructuredUnitobjects:try...exceptblock to attempt standard unit extraction first.StructuredUnitvalues.###Testing
I also added a formal regression test to verify the fix and prevent future regressions. This test specifically targets the scenario where a
StructuredQuantitywith multiple identical units is converted to an unstructuredQuantity.Regression Test Details
File:
astropy/units/tests/test_structured.pyTest Class:
TestStructuredQuantityFunctionsNew Method:
test_structured_to_unstructured_successThe test performs the following checks:
Type Check: Ensures the result of
rfn.structured_to_unstructuredis a standardastropy.units.Quantity.Unit Consistency: Verifies that the resulting quantity inherits the correct base unit (e.g., u.m).
Data Integrity: Validates that the reshaped values match the original data.