Restore cached face_areas fast path in calculate_total_face_area - #1651
Restore cached face_areas fast path in calculate_total_face_area#1651rajeeja wants to merge 2 commits into
Conversation
The HEALPix fast path assigned result but did not return, so it was always overwritten by the recomputed quadrature sum. Restores the else branch and tightens RTOL back to 1e-7.
609ae2c to
7dcb3e0
Compare
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Great catch, thank you! Changes look good. I left a comment to clarify the RTOL choice is mostly arbitrary. You are correct the 1e-7 was causing a warning due to this issue. But, this just pushes the threshold down the line slightly; now there is a warning at 1e-9 but not 1e-8. Should we change the threshold to 1e-8? I think 1e-7 or 1e-6 is fine.
All that to say, "restored 1e-7 RTOL" is misleading; there was nothing to restore, it didn't exist before these changes, it was just the threshold at which errors began. I hope my updated comment clarifies this now. (I usually think every mostly-arbitrary tolerance deserves a comment inline with it, otherwise you come back to it later and worry too much about changing it because you have no idea why it is the number it is.)
|
Also, thank you for taking a close look at #1649 and making this PR to the issue that would have been introduced there! Github was flashing a shiny "want to turn this into a stacked PR?" button at me and I clicked it to see what happened. Now I'm not sure if it was actually worth stacking in this case, since it seems to have the same level of reviewer requirements as merging into main…. Apologies if stacking causes extra confusion here. Though, it maybe it is decent practice with using stacked PRs before applying them to the larger set of changes proposed in #1548 and #1654? |
2 participants
The HEALPix branch assigns
resultbut no longer returns, so it is always overwritten by the recomputed quadrature sum, which drops the exact equal-area values and contradicts the docstring added in this PR. That drift is also why RTOL had to be loosened, so this restores theelsebranch and puts RTOL back to 1e-7. Adds two regression tests; the first fails on the current branch head and passes with the fix.