Add resized area example to resampling documentation

[ludwigVonKoopa]

Adding a resize method in the Scene class, to resize dataset in the resolution wanted.
You specify a size (x, y) in pixels, and resample is done.

This make it people able to easily resize a scene, for example creating a small image to see it faster, when you don't need a full resolution image.

• Closes Add ability to set output image size for Scene.show #368
• Tests added
• Fully documented
• Add your name to AUTHORS.md if not there already

Please let me know if something is missing, or if I should add something else

[djhoese]

How does this differ from the Scene.aggregate method usage: https://satpy.readthedocs.io/en/stable/api/satpy.scene.html#satpy.scene.Scene.aggregate which uses xarray's coarsen.

CC @mraspaud who implemented .aggregate if I remember correctly.

[ludwigVonKoopa]

Correct me if i'm wrong, but aggregate method can only reduce the size by a int coefficient (i.e. scene.aggregate(func="mean", x=2, y=2) which don't allow for example choosing an float coefficient or an exact size (lets say you need a 256x256 image, so you have to delete some pixels to make it fit the wanted size)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.95%. Comparing base (440925d) to head (f73d408).
Report is 378 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2816      +/-   ##
==========================================
- Coverage   95.96%   95.95%   -0.02%     
==========================================
  Files         366      366              
  Lines       53593    53619      +26     
==========================================
+ Hits        51433    51450      +17     
- Misses       2160     2169       +9     

Flag	Coverage Δ
behaviourtests	4.04% <ø> (-0.01%)	⬇️
unittests	96.05% <ø> (-0.02%)	⬇️

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]

Pull Request Test Coverage Report for Build 9498919442

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 29 of 29 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.008%) to 96.048%

---

Totals
Change from base Build 9497248893:	0.008%
Covered Lines:	51593
Relevant Lines:	53716

---

💛 - Coveralls

[djhoese]

Yes, it looks like you are correct. Good point.

I'm torn on how I feel about this, but I'm leaning towards not wanting this added for a couple reasons:

1. This is only a small convenience in my opinion. The logic comes down to 2 Scene methods (finest_area and resample) and a helper on the AreaDefinition (.copy). In general this wouldn't be a huge problem except for my next point...
2. This adds more methods to the Scene which I'd like to avoid if at all possible. As Satpy goes forward and more and more features get added to xarray, we (myself at least) would like the Scene to be come a thin wrapper around xarray's DataTree if not completely replaced by it in future major releases of Satpy. More methods on the Scene means more backwards incompatible changes if we strive for this xarray object based approach in the future and more need for equivalent workflows for users to transition to.
3. We generally regret our usage of **kwargs where it is used in Satpy. It makes things hard to debug and change. This also signals to me that the .resample call is unnecessary in this method as we're simply passing all arguments to the resample call and not using them otherwise.

The overall operation could be simplified to:

new_scn = scn.resample(scn.finest_area().copy(height=x, width=y), ...)

Of course I am not the only maintainer of Satpy so I'd like to hear what some of the others think (@pytroll/satpy-core).

[ludwigVonKoopa]

I understand. I was not aware of the DataTree relation with satpy.

The overall operation could be simplified to:

new_scn = scn.resample(scn.finest_area().copy(height=x, width=y), ...)

Good point.
Maybe instead of adding another method, we could just add this one-liner into the documentation in one of the examples, or the resample page.
It would not add another method, and we would still have this 'tips' somewhere (so we can link this example for sharing purposes, i'm thinking on issue #368).

Or maybe that's too specific, I'm quite new to Pytroll so I lack experience on this project!

[mraspaud]

I agree with @djhoese that adding a new method isn't so nice, in particular because that class is already quite bloated. In the long run, I believe we will have something based off datatree and thus it would be best to have such operations (aggregation, decimation, etc)
in an accessor.
In the meanwhile, documentation about the one-liner is good.

@djhoese we could also start having accessors to the scene object already now, and just make sure they are forward-compatible with datatree, what do you think?

[djhoese]

Yes, I'm ok with have an accessor like interface on the Scene or creating an xarray accessor. However, there is the same bloat issue if this method were added there.

[mraspaud]

Yes, I'm ok with have an accessor like interface on the Scene or creating an xarray accessor. However, there is the same bloat issue if this method were added there.

The accessor I mentioned would be one of many, so that we can separate concerns a bit more. Eg one for spatial transformations (resizing, aggregation, resampling, etc), one for creating composites, etc. Hopefully it wouldn't be so bloated then...

[djhoese]

We should maybe move this discussion to an issue. At some point accessors aren't the right approach and become bloat themselves. A more traditional separate set of functions or classes might be more obvious.

[mraspaud]

We should maybe move this discussion to an issue.

yes

[mraspaud]

LGTM.

[mraspaud]

I'm ok with merging this, but what is the consensus here? Should we just add to the documentation instead?

[ludwigVonKoopa]

I'm ok with not adding another method to the Scene class and just adding a tips in the documentation, but where does it fit the best ?
I'm thinking either :

• writting section, because this tips would allow someone to reduce the size of the image, for saving purpose ?
• resampling for comparison because, resize ~= resampling
• in the example section but this looks like an example too small to fit here

Maybe you have a better idea ?

[djhoese]

What about a new section after this one:

https://satpy.readthedocs.io/en/latest/resample.html#create-custom-area-definition

Something like "Create reduced data area" or...ugh that's a bad section title. Something better than this that isn't too long/wordy.

[ludwigVonKoopa]

Does this kind of example suits you ?

[coveralls]

Pull Request Test Coverage Report for Build 9740461782

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 96.046%

---

Totals
Change from base Build 9702977764:	0.0%
Covered Lines:	51641
Relevant Lines:	53767

---

💛 - Coveralls

[djhoese]

Some comments, but otherwise this seems pretty good.

[djhoese]

It may be worth mentioning that this won't always work with native resampling if it isn't a factor of the original and/or show resampler="nearest" in the code just to suggest that the resampler should be specified.

[ludwigVonKoopa]

Yes i'm adding this in the example to be clearer

[ludwigVonKoopa]

does this changes suits you ?

(some tests don't pass, but I don't think they are relative to my modifications as... i'm only modifying some comments)

FAILED satpy/tests/test_modifiers.py::TestPSPRayleighReflectance::test_rayleigh_corrector[B01-wavelength0-1000-rayleigh_only-70-95-1-41.540239-exp_unique0] - tarfile.ReadError: file could not be opened successfully

[ludwigVonKoopa]

correction : after rebase with main, no more unsuccessful checks

[coveralls]

Pull Request Test Coverage Report for Build 9944227638

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 8 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.02%) to 96.054%

---

Files with Coverage Reduction	New Missed Lines	%
satpy/tests/utils.py	2	93.16%
satpy/tests/reader_tests/gms/test_gms5_vissr_l1b.py	3	98.67%
satpy/tests/reader_tests/gms/test_gms5_vissr_navigation.py	3	97.18%

Totals
Change from base Build 9842330086:	-0.02%
Covered Lines:	51679
Relevant Lines:	53802

---

💛 - Coveralls

[djhoese]

My warning got a little bigger than I expected, but I think it is more clear. Maybe I should have just linked to the information about the native resampler... 🤔

[djhoese]

Maybe I should have just linked to the information about the native resampler...

Of course the native resampler documentation doesn't mention this 🤦‍♂️

https://satpy.readthedocs.io/en/stable/api/satpy.resample.html#satpy.resample.NativeResampler

[adybbroe]

LGTM