DRAFT: HEALPix Area Correction#1232
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates HEALPix support by deriving pixel areas directly in the grid and by adding a new parameter to control boundary computation in UxDataset.from_healpix. Key changes include:
- In uxarray/grid/grid.py: Short-circuits area computation for HEALPix grids using the n_side attribute.
- In uxarray/core/dataset.py: Adds a new "pixels_only" parameter to from_healpix to control whether boundaries are computed.
- In test/test_healpix.py: Introduces tests for verifying the derived HEALPix pixel areas and for asserting boundary inclusion when pixels_only is false.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| uxarray/grid/grid.py | Updates compute_face_areas for HEALPix to return a derived face area. |
| uxarray/core/dataset.py | Adds a pixels_only parameter to UxDataset.from_healpix and passes it to Grid.from_healpix. |
| test/test_healpix.py | Adds tests to verify HEALPix area derivation and correct boundary parameter behavior. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new feature for HEALPix grids by deriving pixel areas using a formula and introduces a missing "pixels_only" parameter to UxDataset.from_healpix().
- In uxarray/grid/grid.py the HEALPix pixel area is derived using a presumed formula.
- In uxarray/core/dataset.py the function signature of from_healpix() has been updated to include the pixels_only parameter.
- In test/test_healpix.py new tests verify the computed HEALPix pixel area and boundary construction when pixels_only is toggled.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| uxarray/grid/grid.py | Adds HEALPix area computation logic to compute_face_areas() |
| uxarray/core/dataset.py | Updates from_healpix() to accept a pixels_only parameter |
| test/test_healpix.py | Adds tests verifying HEALPix area computation and boundary presence |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR derives the HEALPix pixel area within the grid class and adds a new parameter to the HEALPix dataset loader so that users can choose whether to only compute pixel centers or also construct boundaries.
- Derives HEALPix area using the formula Ω_pix = π / (3 * n_side²)
- Adds the "pixels_only" parameter to both Grid.from_healpix and UxDataset.from_healpix and updates the corresponding tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| uxarray/grid/grid.py | Adds HEALPix area derivation logic in compute_face_areas() |
| uxarray/core/dataset.py | Updates from_healpix() to accept a pixels_only parameter |
| test/test_healpix.py | Introduces tests validating HEALPix area computation and boundary setup |
Comments suppressed due to low confidence (1)
uxarray/core/dataset.py:285
- Update the docstring to clearly document the new 'pixels_only' parameter and its effect on boundary computations.
def from_healpix(cls, ds: Union[str, os.PathLike, xr.Dataset], pixels_only: bool = True, **kwargs):
erogluorhan
left a comment
erogluorhan
left a comment
There was a problem hiding this comment.
Please see my comments this PR trying to derive areas according to HEALPix without actually changing the edge topology. Maybe for now:
- Hold off on deriving the areas for HEALPix like it is done in this PR
- Create an issue about the face areas currently not being equal
- Maybe also discuss things like whether the edge topology generation should be changed for HEALPix? This issue would be something good to discuss during the upcoming hackathon.
| # Górski et al. 2005, “HEALPix: A Framework for High‐Resolution Discretization and Fast Analysis of Data Distributed on the Sphere” | ||
| n_side = self._ds.attrs["n_side"] | ||
| face_area = np.pi / (3 * n_side**2) | ||
| return np.ones(self.n_face) * face_area, None |
There was a problem hiding this comment.
While I get the reasoning about this for achieving actual equal-area faces for HEALPix, I am a bit concerned about deriving HEALPix area as in here:
- Actually we are not generating our edges in the topology in the same way as HEALPix does (I mean the actual topology, not visualization), right? So, each pixel's edges being generated in our way shouldn't be resulting in equal areas as it is being attempted to derived here .
- We are kind of trying to hard-code the area values now in here.
- Where would this be a problem?
- Probably where an operator uses both edge information and area information.
|
|
||
| n_side = uxgrid._ds.attrs["n_side"] | ||
| expected_area = np.ones(uxgrid.n_face) * np.pi / (3 * n_side**2) | ||
| assert np.array_equal(uxgrid.face_areas.values, expected_area) |
There was a problem hiding this comment.
See my comments on the area derivation part in grid.py
philipc2
changed the title
UxDataset.from_healpix()
|
Didn't go through the change, but for a HEALPix mesh the area of each pixel is predetermined and equal across the entire sphere. We don't need to compute it using our routines, it can be a simple formula and we can bypass our routines.
|
That's correct. This is what I'm looking to implement here. |
|
I'm happy to close this for now and keep #1236 open, unless you envision us making any of these changes in the near future. |
Yeah, that works with me! |
Closes #1236
Overview