spacetelescope / spherical_geometry

A Python package for handling spherical polygons that represent arbitrary regions of the sky
http://spherical-geometry.readthedocs.io/
61 stars 31 forks source link

Fix crash in multi-union for nearly identical input polygons #233

Closed mcara closed 1 year ago

mcara commented 1 year ago

Closes #232 Closes #209

Both #232 and #209 report the same error that occasionally may happen when computing a multi_union(). #232 reports that this is happening when some of the input polygons are nearly identical (having very close to each other vertices).

This PR, while not a real fix to the root-cause of the bug (or likely accuracy limitation) in the graph code, it provides a workaround that could help while the graph code is being investigated.

hbushouse commented 1 year ago

Why won't the CI tests run?

nden commented 1 year ago

The CI needed a manual restart. It was disabled because of inactivity for more than 60 days.

nden commented 1 year ago

Tests are failing

mcara commented 1 year ago

@nden fixed

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 91.66% and project coverage change: +0.59% :tada:

Comparison is base (4a27b3b) 65.09% compared to head (d841821) 65.68%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #233 +/- ## ========================================== + Coverage 65.09% 65.68% +0.59% ========================================== Files 8 8 Lines 1252 1262 +10 ========================================== + Hits 815 829 +14 + Misses 437 433 -4 ``` | [Files Changed](https://app.codecov.io/gh/spacetelescope/spherical_geometry/pull/233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | |---|---|---| | [spherical\_geometry/polygon.py](https://app.codecov.io/gh/spacetelescope/spherical_geometry/pull/233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-c3BoZXJpY2FsX2dlb21ldHJ5L3BvbHlnb24ucHk=) | `82.46% <91.66%> (+0.40%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/spacetelescope/spherical_geometry/pull/233/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nden commented 1 year ago

Can you add a test? Perhaps one of the examples in the tickets.

mcara commented 1 year ago

It would be nice to indicate what the current tolerance corresponds to in arcseconds in the event that HST7 has sufficient resolution to care.

I hope the code in the graph could be fixed or significantly improved by the time HST7 is launched.

mcara commented 1 year ago

@perrygreenfield Please verify these handwaving calculations:

Assuming dx is along a circle and dy=dz=0, 5e-9 is in radians (when dx is small), and so, converting this to acrseconds:

(180/pi)*3600*5e-9 = 0.0010313 arcsec

If all directions are allowed for errors (dy!=0 and dz!=0), then we could increase the above estimate by sqrt(3)<2 or so. Then, let's say if vertices are closer to each other by less than 0.002 arcsec, then we will consider two polygons to be the same polygon.

In terms of JWST pixel scale of 0.11 arcsec, this is 1/50 of a pixel and on the surface of the Earth (mean R), this would be equivalent to two vertices being closer than about 3 mm.

Does this seem reasonable?

perrygreenfield commented 1 year ago

Yes, that seems reasonable, though the combined error would be more like sqrt(2) larger than for one dimension (the three coordinates are constrained to be on the surface of a sphere). Actually, this may be large enough for HST2 or HST3 to worry about. Hopefully you will fix the bug by then (they being multiple decades away).

mcara commented 1 year ago

Yes, that seems reasonable, though the combined error would be more like sqrt(2) larger than for one dimension (the three coordinates are constrained to be on the surface of a sphere).

I agree. I just wanted to be very conservative.

mcara commented 1 year ago

Actually, this may be large enough for HST2 or HST3 to worry about. Hopefully you will fix the bug by then (they being multiple decades away).

My hope was that you were the last person to look at the multi_union and Graph code... 🤞

mcara commented 1 year ago

Based on @perrygreenfield comment, adjusted tolerance estimate is:

If vertices are closer to each other by less than 0.0015 arcsec, then we will consider two polygons to be the same polygon.

In terms of JWST pixel scale of 0.11 arcsec, this is 1/75 of a pixel and on the surface of the Earth (mean R), this would be equivalent to two vertices being closer than about 2 mm.

mcara commented 1 year ago

JWST pipeline regression tests are running here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/814/

nden commented 1 year ago

Should anything be added to the documentation, explaining the choice of the tolerance value?