Repository navigation
Conversation
Contributor
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
Comment on lines
+693
to
+695
| face_areas = self.uxgrid.compute_face_areas( | ||
| quadrature_rule=quadrature_rule, order=order, as_uxarray=True | ||
| ) |
Contributor
Author
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.
rajeeja
force-pushed
the
rajeeja/integrate-quadrature
branch
from
October 27, 2026 00:16
ba9e5c5 to
a546ed8
Compare
rajeeja
marked this pull request as ready for review
October 27, 2026 20:14
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.

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