Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Dataset integration suppresses quadrature validation errors, returning an empty dataset with a misleading warning.
1 open issue
What changed in this PR
Restores quadrature selection in UxDataArray.integrate() to address #1599 while preserving cached face areas for default settings.
Changes:
- Computes fresh face areas for non-default quadrature rules or orders.
- Documents quadrature options and HEALPix behavior.
- Adds tests for parameter handling, default results, and lazy integration.
| File | Description |
|---|---|
uxarray/core/dataset.py |
Clarifies quadrature parameter documentation. |
uxarray/core/dataarray.py |
Honors quadrature arguments and documents area selection. |
test/grid/integrate/test_basic.py |
Adds quadrature and lazy-execution regression tests. |
Review effort: Balanced
| face_areas = self.uxgrid.compute_face_areas( | ||
| quadrature_rule=quadrature_rule, order=order, as_uxarray=True | ||
| ) |
There was a problem hiding this comment.
Good catch — DataCenteringError and DimensionError both inherit from ValueError, so the broad handler also swallowed quadrature validation failures. Narrowed the catch to those two expected skip cases and added a dataset-level regression test asserting that an invalid order propagates. Fixed in ba9e5c5; the focused integration tests pass.
Both kwargs were typed optional and ignored, so callers passing None got the default result. Tightening the annotations turned that into a ValueError from the quadrature validation; map None back to the defaults instead.
ba9e5c5 to
a546ed8
Compare
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Thank you for tackling this one! Just had a few minor inline comments here.
| def test_integrate_default_uses_cached_face_areas(gridpath): | ||
| """The default rule/order must still reuse the cached ``face_areas``.""" | ||
| uxgrid = ux.open_grid(gridpath("ugrid", "outCSne30", "outCSne30.ug")) | ||
| uxda = ux.UxDataArray( | ||
| data=np.ones(uxgrid.n_face), dims=("n_face",), uxgrid=uxgrid, name="ones" | ||
| ) | ||
|
|
||
| expected = np.einsum("i,...i", uxgrid.face_areas.values, uxda.values) | ||
| nt.assert_array_equal(uxda.integrate().values, expected) | ||
| nt.assert_array_equal( | ||
| uxda.integrate(quadrature_rule="triangular", order=4).values, expected | ||
| ) |
There was a problem hiding this comment.
This doesn't actually test what the docstring claims; it could be internally re-computing face_areas and this test wouldn't complain. That said, I'm not actually convinced that a test of "make sure you are using the cached face_areas" needs to be added here, it's fine to just test the values' correctness; this test could be removed entirely.
| @pytest.mark.parametrize( | ||
| "kwargs", | ||
| [ | ||
| {"quadrature_rule": None}, | ||
| {"order": None}, | ||
| {"quadrature_rule": None, "order": None}, | ||
| ], | ||
| ) | ||
| def test_integrate_none_means_default(gridpath, kwargs): | ||
| """``None`` keeps selecting the default rule/order. | ||
|
|
||
| Both kwargs were typed optional and ignored, so callers passing ``None`` | ||
| got the default result. Honoring the kwargs must not turn that into an | ||
| error. | ||
| """ |
There was a problem hiding this comment.
Is this test really necessary? This is basically just saying "make sure None is really the default", which feels almost tautological; the only thing really being tested here is the function signature itself. I don't think the test suite should necessarily be checking something like that.
| def _integration_face_areas(uxgrid, quadrature_rule, order): | ||
| """Face areas for ``integrate``; raises ValueError on a bad rule/order.""" |
There was a problem hiding this comment.
This small helper function almost certainly belongs in uxarray/grid/area.py, not the top of dataarray.py.
Closes #1599
Overview
integrate()acceptedquadrature_ruleandorderbut ignored them: #1379 switched to the cachedface_areasproperty, which is always triangular/order 4. Reuse the cache only for those defaults and compute fresh areas otherwise, matchingcalculate_total_face_area().Nonekeeps selecting the default for both kwargs, as it did when they were ignored.UxDataset.integrate()now computes areas once and raises on a bad rule/order instead of skipping every variable.PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rstAI Disclosure
AI Usage: Claude Code