search

pytroll / satpy

Python package for earth-observing satellite data processing
http://satpy.readthedocs.org/en/latest/
GNU General Public License v3.0
1.04k stars 287 forks source link

add resize method for Scene class #2816

Open ludwigVonKoopa opened 2 weeks ago

ludwigVonKoopa commented 2 weeks ago

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.

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

djhoese commented 2 weeks ago

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 commented 2 weeks ago

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[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 95.94%. Comparing base (3c7938c) to head (0243a60). Report is 72 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2816 +/- ## ======================================= Coverage 95.94% 95.94% ======================================= Files 366 366 Lines 53504 53584 +80 ======================================= + Hits 51332 51412 +80 Misses 2172 2172 ``` | [Flag](https://app.codecov.io/gh/pytroll/satpy/pull/2816/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | Coverage Δ | | |---|---|---| | [behaviourtests](https://app.codecov.io/gh/pytroll/satpy/pull/2816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `4.04% <ø> (-0.01%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/pytroll/satpy/pull/2816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `96.04% <ø> (+<0.01%)` | :arrow_up: | 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 2 weeks ago

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.

Details

Totals Coverage Status
Change from base Build 9497248893: 0.008%
Covered Lines: 51593
Relevant Lines: 53716
💛 - Coveralls
djhoese commented 1 week ago

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 commented 1 week ago

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 https://github.com/pytroll/satpy/issues/368).

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

mraspaud commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

We should maybe move this discussion to an issue.

yes

mraspaud commented 1 week ago

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

ludwigVonKoopa commented 1 week ago

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 :

Maybe you have a better idea ?

djhoese commented 1 week ago

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 commented 9 hours ago

Does this kind of example suits you ?

coveralls commented 7 hours ago

Pull Request Test Coverage Report for Build 9740461782

Details

Totals Coverage Status
Change from base Build 9702977764: 0.0%
Covered Lines: 51641
Relevant Lines: 53767
💛 - Coveralls