Fix HEALPix face area calculation to enforce equal area property

[rajeeja]

Address #1236 and update face area calculation API

[erogluorhan]

Implementation looks good except a few inline comments and the following:

Per today's discussion:

• Did you have a chance to look into how the current UXarray area calculation behaves between coarse and fine resolutions, compared to the HEALPIx areas at those levels?
• Could you please have a look into where in the documentation we could make some clear statements about how our geometric representation of HEALPix pixels (i.e. GCAs) differ from their own algorithmic geometry, and how we did this area adjustments because of that?

[rajeeja]

Implementation looks good except a few inline comments and the following:

Per today's discussion:

• Did you have a chance to look into how the current UXarray area calculation behaves between coarse and fine resolutions, compared to the HEALPIx areas at those levels?
• Could you please have a look into where in the documentation we could make some clear statements about how our geometric representation of HEALPix pixels (i.e. GCAs) differ from their own algorithmic geometry, and how we did this area adjustments because of that?

Resolution 0 (coarse): 20.5% max error, 100% faces affected Resolution 7 (fine): 10.0% max error, 0.01% faces affected Overall we can say that mean errors decrease dramatically, but max errors persist

Based on my analysis of where these max errors occur, I can see

1. HEALPix's 12 base pixels (0 resolution) have fundamentally different shapes
2. Equatorial pixels: More "square-like" in HEALPix's projection
3. Mid-latitude pixels: More "diamond-like" or distorted
4. UXarray's GCA representation doesn't perfectly capture these shape differences

These errors are systematic are related to how different HEALPix pixel types (equatorial vs mid-latitude) are represented using Great Circle Arcs. This is a fundamental geometric representation issue, not a numerical precision problem.

So, the area property in this PR preserves HEALPix's equal-area property!

[rajeeja]

What do you think about adding documentation and a cross-reference in area_calc.ipynb and healpix.ipynb?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[erogluorhan]

What do you think about adding documentation and a cross-reference in area_calc.ipynb and healpix.ipynb?

I think this makes sense in such a way:

• area_calc.ipynb: Don't change the generic text with HEALPix-specific info at all, but maybe consider only adding a warning admonition that speaks about the special case of HEALPix areas and refer reader to the healpix.ipynb
• In healpix.ipynb, talk about the difference between HEALPix geometry and our representation of it (as you did in the previous comment here), and then talk about its potential implications, our adjustments to the area calculations, etc.

Makes sense?

[erogluorhan]

Reviewing this PR's latest revisions on healpix.ipynb made me notice something and further think about how to deliver this area adjustment in the best possible (least confusing?) way:

• With the added parts, the "Working with HEALPix" notebook now discusses Best Practices, i.e. about using the face_areas property and avoiding the compute_face_areas function when it is a HEALPix.
  • This might easily be overlooked or forgotten by the user though; hence it could lead confusions. Then I started to think why we should provide a compute_face_areas API even though we have a face_areas API already. Why don't we have it only as an internal API?
    • Then I dug into finding where compute_face_areas has been used so far. It is used in:
      • grid.calculate_total_face_area,
      • dataarray.integrate, and
      • dataset.integrate.
    • Also, none of them seems to have been modified in this PR to use face_areas to leverage the HEALPix area adjustment.

I'd suggest that:

• We convert compute_face_areas into internal API
  • Relatedly, remove its references from the docs, tests, etc.
  • Also relatedly, revise healpix.ipynb again to only mention how face_areas is adjusted when it is a HEALPix grid (after discussing the systematic difference between UXarray and HEALPix as you already did)
• Turn compute_face_areas usages mentioned above into face_areas instead
• Likewise, create compute_healpix_face_areas as an internal API, not user API

Do these make sense?

[rajeeja]

Reviewing this PR's latest revisions on healpix.ipynb made me notice something and further think about how to deliver this area adjustment in the best possible (least confusing?) way:

• With the added parts, the "Working with HEALPix" notebook now discusses Best Practices, i.e. about using the face_areas property and avoiding the compute_face_areas function when it is a HEALPix.

  • This might easily be overlooked or forgotten by the user though; hence it could lead confusions. Then I started to think why we should provide a compute_face_areas API even though we have a face_areas API already. Why don't we have it only as an internal API?

    • Then I dug into finding where compute_face_areas has been used so far. It is used in:

      • grid.calculate_total_face_area,
      • dataarray.integrate, and
      • dataset.integrate.

    • Also, none of them seems to have been modified in this PR to use face_areas to leverage the HEALPix area adjustment.

I'd suggest that:

• We convert compute_face_areas into internal API

  • Relatedly, remove its references from the docs, tests, etc.
  • Also relatedly, revise healpix.ipynb again to only mention how face_areas is adjusted when it is a HEALPix grid (after discussing the systematic difference between UXarray and HEALPix as you already did)

• Turn compute_face_areas usages mentioned above into face_areas instead

• Likewise, create compute_healpix_face_areas as an internal API, not user API

Do these make sense?

Good observation/plan and I agree with most of proposed changes. It did look cumbersome while doing this notebook.
A couple of minor concerns - what if users need to force recomputation of area for some workflow? and maybe some backward compatibiluty issues.

[erogluorhan]

Good observation/plan and I agree with most of proposed changes. It did look cumbersome while doing this notebook. A couple of minor concerns - what if users need to force recomputation of area for some workflow? and maybe some backward compatibiluty issues.

Yeah, that's a good point!

Thinking out loud: How about making all the suggested changes above in my comment,but adding an admonition into the HEALPix notebook about its workflow possibility and guiding the reader about an internal function (_compute_face_areas()) that could still be used for that purpose?

[rajeeja]

Good observation/plan and I agree with most of proposed changes. It did look cumbersome while doing this notebook. A couple of minor concerns - what if users need to force recomputation of area for some workflow? and maybe some backward compatibiluty issues.

Yeah, that's a good point!

Thinking out loud: How about making all the suggested changes above in my comment,but adding an admonition into the HEALPix notebook about its workflow possibility and guiding the reader about an internal function (_compute_face_areas()) that could still be used for that purpose?

Done.

[erogluorhan]

Thanks very much for the updates; this is getting there! Please see below and other comments:

• The HEALPix notebook needs to be updated per yesterday's discussion after today's updates.
• Update user API doc to remove compute_face_areas.

[erogluorhan]

Looks great; thanks a lot for this fix!