Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AreaDefinition array index methods mishandling outer edge values #596

Merged
merged 5 commits into from
Jul 22, 2024

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Apr 26, 2024

An attempt to fix ssec/polar2grid#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).

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff
  • Fully documented

@djhoese djhoese added the bug label Apr 26, 2024
@djhoese djhoese requested review from mraspaud and pnuu April 26, 2024 02:50
@djhoese djhoese self-assigned this Apr 26, 2024
Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Just need to restart the CI jobs that were cancelled.

@pnuu
Copy link
Member

pnuu commented Jun 20, 2024

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

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@djhoese
Copy link
Member Author

djhoese commented Jun 20, 2024

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
Copy link
Member Author

djhoese commented Jun 20, 2024

@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?

Unnecessary for unstable testing
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 93.97% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

Coverage Status

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

@djhoese djhoese requested a review from mraspaud June 20, 2024 18:50
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, I think this looks reasonable. I assuming the existing test cover other user cases, and since tests pass, feel free to merge!

@djhoese
Copy link
Member Author

djhoese commented Jul 22, 2024

Right. The only test that didn't pass was the one changed in this PR where it specifically checked that values under the mask (where mask is True) remained valid-ish.

@djhoese djhoese merged commit 3651ce9 into pytroll:main Jul 22, 2024
26 of 27 checks passed
@djhoese djhoese deleted the bugfix-area-indices branch July 22, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems running geo2grid.sh on RadM1 imagery from 08Apr2024?
5 participants