pytroll / pyresample

Geospatial image resampling in Python
http://pyresample.readthedocs.org
GNU Lesser General Public License v3.0
343 stars 95 forks source link

Fix AreaDefinition array index methods mishandling outer edge values #596

Open djhoese opened 2 months ago

djhoese commented 2 months ago

An attempt to fix https://github.com/ssec/polar2grid/issues/691

Basically, current bounding operations via get_area_slices have trouble when the resulting lon/lat extents match almost exactly with the source area definition. This PR should fix a couple important bugs in the array index retrieval methods of the AreaDefinition:

  1. Floating point precision issues where a X coordinate that is just outside the area (a float precision different) can still be considered part of the area (not masked).
  2. Not including -0.5 to 0.0 pixel indexes as "0" (it is the left most part of the left most pixels.
  3. Including an extra 0.5 pixels on the right of the area.

Locally I see one failure where my particular approach has changed the underlying value for elements that are masked and this test specifically checks that that doesn't happen (they stay unchanged).

pnuu commented 4 weeks ago

I merged with main to trigger the tests, there were no "re-run failed jobs" available anymore.

djhoese commented 3 weeks ago

I'm surprised there is no test accompanying the change? That would make me understand the problem better I suppose :)

Are you telling me you expect me to finish my PRs before you review them? And why don't you remember every conversation you've ever had?

Yes...I didn't realize how incomplete this PR was. I just remember we didn't decide on how it should work. I'll see if I can finish it today.

djhoese commented 3 weeks ago

@mraspaud Tests refactored and added. The bottom line is that you (the user) are not guaranteed values outside of the area that are masked are valid underneath the mask. This is the main question.

For other similar methods like get_lonlat_from_array_indices, no masking is done no matter what. You can request coordinates outside of the AreaDefinition. The special thing about the method in question here is that it is returning array indices, not coordinates. There's get_array_coordinates_from_lonlat for that case. So...what are we allowed to do with masked values?

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.97%. Comparing base (56f4506) to head (8feba45). Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #596 +/- ## ========================================== - Coverage 94.01% 93.97% -0.04% ========================================== Files 92 86 -6 Lines 13836 13753 -83 ========================================== - Hits 13008 12925 -83 Misses 828 828 ``` | [Flag](https://app.codecov.io/gh/pytroll/pyresample/pull/596/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/pytroll/pyresample/pull/596/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `93.97% <100.00%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll#carryforward-flags-in-the-pull-request-comment) to find out more.

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

coveralls commented 3 weeks ago

Coverage Status

coverage: 93.679% (+0.003%) from 93.676% when pulling 8feba45ae39bd5e04f569dcbb41fa9c131229b19 on djhoese:bugfix-area-indices into db94a87b21bfff43eecb71458f54c574dd344833 on pytroll:main.