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

Added resolution to SwathDefinition in slice.py #650

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nilssonjacob
Copy link

Adds resolution to area if the area to contain is of type SwathDefinition to avoid crashing.

@djhoese
Copy link
Member

djhoese commented Mar 10, 2025

I could be wrong and @mraspaud or @pnuu may have to correct me but I think the slicer stuff is not meant to support SwathDefinitions by design. I could be forgetting what this code is used for though so forgive me if so. A swath is not guaranteed to have contiguous pixels and therefore can't/shouldn't be sliced by geolocation/lon/lat bounds.

@djhoese
Copy link
Member

djhoese commented Mar 10, 2025

Sorry, I should have also said thank you for making this PR. Even if I am right this should start the discussion to supporting what you're trying to do and is probably a discussion that we need to have.

Copy link

codecov bot commented Mar 11, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.97%. Comparing base (98f20a7) to head (a97a668).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pyresample/slicer.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
- Coverage   93.98%   93.97%   -0.01%     
==========================================
  Files          86       86              
  Lines       13543    13545       +2     
==========================================
+ Hits        12728    12729       +1     
- Misses        815      816       +1     
Flag Coverage Δ
unittests 93.97% <66.66%> (-0.01%) ⬇️

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.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pnuu
Copy link
Member

pnuu commented Mar 11, 2025

Looking at the diff, the only actual change is the addition of the resolution. The other bits are just reformatting. So SwathDefinition is already somewhat supported.

I think the addition of the resolution is in the wrong place. This is now done to AreaSlicer which handles AreaDefinitions. SwathSlicer should be used for SwathDefinitions.

Also, I'd say the resolution shouldn't be hard-coded to (1, 1) kilometers. Via Satpy we might get some information on the resolution from the reader/file handler, but maybe not in every case.

@djhoese
Copy link
Member

djhoese commented Mar 11, 2025

Maybe we should take a step back. @nilssonjacob what is your use case? What code are you running that isn't working?

@coveralls
Copy link

Coverage Status

coverage: 93.671% (-0.006%) from 93.677%
when pulling a97a668 on nilssonjacob:main
into b77d0b6 on pytroll:main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants