Refactor resampler module to submodules #3124

pnuu · 2025-05-12T14:45:06Z

This PR refactors the satpy/resample.py module to a satpy.resample subpackage and splits the contents to several modules.

At least the warnings in the tests should be fixed, but this is the first version to see if there are other things to consider. Also some of the imports could be moved to where they are used. And the docstring should be fixed with the new import paths.

Triggered by #3123

Closes #xxxx
Tests added
Fully documented
Add your name to AUTHORS.md if not there already

satpy/resample/__init__.py

djhoese · 2025-05-12T17:16:44Z

The other thing about this PR that I'm realizing is that some of the resamplers (ewa maybe? kdtree nearest maybe?) exist in pyresample in a way that should be useable. Or at least that was my intention before I stopped working on pyresample 2.0. Maybe it never got all the way there (as we already know I'm wrong about the caching functionality implemented in pyresample).

codecov · 2025-05-13T07:33:50Z

Codecov Report

Attention: Patch coverage is 90.72643% with 60 lines in your changes missing coverage. Please review.

Project coverage is 96.23%. Comparing base (55328e2) to head (bfdb334).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
satpy/resample/kdtree.py	87.65%	20 Missing ⚠️
satpy/resample/base.py	81.73%	19 Missing ⚠️
satpy/resample/native.py	91.96%	9 Missing ⚠️
satpy/resample/bucket.py	94.94%	5 Missing ⚠️
satpy/area_utils.py	75.00%	3 Missing ⚠️
satpy/coords_utils.py	94.00%	3 Missing ⚠️
satpy/resample/__init__.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3124      +/-   ##
==========================================
- Coverage   96.24%   96.23%   -0.01%     
==========================================
  Files         421      429       +8     
  Lines       57671    57777     +106     
==========================================
+ Hits        55506    55603      +97     
- Misses       2165     2174       +9

Flag	Coverage Δ
behaviourtests	`3.84% <6.02%> (-0.08%)`	⬇️
unittests	`96.32% <90.72%> (-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.

coveralls · 2025-05-13T07:48:23Z

Pull Request Test Coverage Report for Build 15308592336

Details

589 of 652 (90.34%) changed or added relevant lines in 29 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 96.335%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
satpy/area_utils.py	10	13	76.92%
satpy/coords_utils.py	48	51	94.12%
satpy/resample/init.py	6	10	60.0%
satpy/resample/bucket.py	94	99	94.95%
satpy/resample/native.py	103	112	91.96%
satpy/resample/base.py	86	105	81.9%
satpy/resample/kdtree.py	142	162	87.65%

Totals
Change from base Build 15307805052:	-0.01%
Covered Lines:	55887
Relevant Lines:	58013

💛 - Coveralls

pnuu · 2025-05-13T09:41:57Z

I think this is now ready. Unless we come up with a better name for the base module.

djhoese

One more comment for making things private, but also what do you think about moving get_area_file/def functions and add_xy_coords and add_crs_xy_coords to a utils.py module? Or even go as far as the get_area_* functions to one module outside of the resample subpackage (since they are used by readers and users) like satpy.area or satpy.area_utils or satpy.area_config? And add_*_coords to another module inside or outside of resampling? Like satpy.coords or satpy.coords_utils?

satpy/resample/__init__.py

djhoese · 2025-05-13T15:03:29Z

Oh another thought: if we imagine that the base resampler class(es) will all end up moving to pyresample and possibly even the "prepare_resampler" or "get_resampler"-style functions would be moved. Does that change the naming? Or rather, maybe those types of functions should not be in base.py? If the base class went away then these helper/utility functions would be in a base.py when they don't do anything "base"-like.

pnuu · 2025-05-16T12:07:43Z

One more comment for making things private, but also what do you think about moving get_area_file/def functions and add_xy_coords and add_crs_xy_coords to a utils.py module? Or even go as far as the get_area_* functions to one module outside of the resample subpackage (since they are used by readers and users) like satpy.area or satpy.area_utils or satpy.area_config? And add_*_coords to another module inside or outside of resampling? Like satpy.coords or satpy.coords_utils?

I'll have a look at this today evening after I reach the hotel in Milan, where I need to stay overnight due to a cancelled flight.

djhoese

Awesome. I noticed one small thing that could be changed and cleaned up. The only other discussion we should maybe have is how many satpy/X_utils.py modules do we have before making things a series of utils/X.py modules? Or would it be better to drop the _utils.py part of the name and just call them satpy/area.py and satpy/coords.py?

satpy/coords_utils.py

djhoese

Awesome! Just. Awesome.

I think the most obvious win from this PR is how many things are loading from the area and coords utils instead of the resample module/subpackage.

I made a couple inline comments about long term design decisions. The only other remaining question for me is should satpy/area_utils.py and coords_utils.py have the _utils removed?

djhoese · 2025-05-29T19:02:41Z

satpy/area_utils.py

+    if paths:
+        return paths
+    else:
+        return get_config_path("areas.def")


In a near-future PR should we drop support for the old areas format?

Yeah, I think it's well due time. At the latest in 1.0.

djhoese · 2025-05-29T19:06:20Z

satpy/resample/base.py

+    try:
+        from .native import get_native_resampler_classes
+        resamplers.update(get_native_resampler_classes())
+    except ImportError:
+        pass


I don't think it needs to be done in this PR but this seems like another case where it would be easy to have a dictionary or list of strings to import and use importlib.import_module and getattr to get access to them. You could also use with suppress(ImportError): to avoid the "uglier" except ImportError.

I actually considered with suppress(): for a sec yesterday, but got lazy. If I don't have anything better to do tomorrow, I'll have another look at both options.

djhoese · 2025-05-29T19:07:59Z

satpy/resample/ewa.py

+def get_ewa_resampler_classes():
+    """Get bucket resampler classes."""
+    return {
+        "ewa": DaskEWAResampler,
+        "ewa_legacy": LegacyDaskEWAResampler,
+    }


Is the idea that this entire module would go away if/when Resamplers can be loaded directly from a pyresample interface? Or should something like this be replace with a dict/list of strings like I mentioned in the base.py comment? That is a series of strings to be imported.

The idea was to just kind of follow the structure of the other resample modules and provide a single function to get a dict of the available resamplers to replace the RESAMPLERS dict in __init__.py but still have the ability to get all of them.

djhoese · 2025-05-29T19:10:51Z

satpy/scene.py

 from satpy.composites import IncompatibleAreas
 from satpy.composites.config_loader import load_compositor_configs_for_sensors
 from satpy.dataset import DataID, DataQuery, DatasetDict, combine_metadata, dataset_walker, replace_anc
 from satpy.dependency_tree import DependencyTree
 from satpy.node import CompositorNode, MissingDependencies, ReaderNode
 from satpy.readers.core.loading import load_readers
-from satpy.resample import get_area_def, prepare_resampler, resample_dataset
+from satpy.resample.base import prepare_resampler, resample_dataset


Since pyresample is a hard requirement this probably doesn't matter, but I'm wondering if this import should be in-function to avoid import Resampler classes. 🤷‍♂️ Eh that will probably go away naturally when more things are moved to pyresample.

Yes, certainly. I'll move it in-function tomorrow if I don't have anything else. Would help in the short-term anyways.

Refactor resampler module to submodules

e873d3b

pnuu self-assigned this May 12, 2025

pnuu requested review from djhoese and mraspaud as code owners May 12, 2025 14:45

pnuu added component:resampling cleanup Code cleanup but otherwise no change in functionality labels May 12, 2025

lahtinep added 2 commits May 12, 2025 17:54

Update resample imports

b65098a

Update and move resampling docstring

f3712ab

djhoese reviewed May 12, 2025

View reviewed changes

satpy/resample/__init__.py Outdated Show resolved Hide resolved

lahtinep added 2 commits May 13, 2025 10:26

Fix DOS line-endings, fix get_area_def import

0b59ca8

Fix imports to use new resampler modules

9051519

lahtinep added 2 commits May 13, 2025 10:42

Simplify prepare_resampler()

41f5ef0

Simplify add_xy_coords()

43f4b33

lahtinep added 7 commits May 13, 2025 11:11

Simplify BucketResamplerBase.resample()

80b16e1

Simplify KDTreeResampler.load_neighbour_info()

b16e834

Simplify NativeResampler.compute()

d68afb5

Simplify native._rechunk_if_nonfactor_chunks()

ea5d24b

Simplify _check_crs_units and add_xy_coords() more

c71f33d

Reduce cyclomatic complecity of NativeResampler._expand_reduce()

86a4def

Make base.hash_dict a private function

44ee39b

pnuu added this to PCW Spring 2025 May 13, 2025

pnuu moved this to In Progress in PCW Spring 2025 May 13, 2025

djhoese requested changes May 13, 2025

View reviewed changes

satpy/resample/__init__.py Outdated Show resolved Hide resolved

pnuu mentioned this pull request May 13, 2025

Refactor modules to avoid unnecessary imports #3123

Open

6 tasks

pnuu moved this from In Progress to Ready for review in PCW Spring 2025 May 14, 2025

lahtinep added 2 commits May 16, 2025 14:44

Make get_area_file and update_resampled_coords private functions

e33ea4b

Fix import

412d848

lahtinep added 5 commits May 16, 2025 20:26

Move get_area_def and get_area_file to a new module satpy.area_utils

d7e4e9e

Create satpy.coords_utils module

125e691

Fix get_area_def patching

f24507f

Merge branch 'main' into refactor-resample

28e02ca

Fix bucket doc references, again

9371f45

djhoese requested changes May 18, 2025

View reviewed changes

satpy/coords_utils.py Outdated Show resolved Hide resolved

pnuu and others added 6 commits May 28, 2025 18:53

Merge branch 'main' into refactor-resample

5a73b6e

Use helper function instead of if/elif/else in satpy.resample imports

e5a4db7

Fix resample imports and doc references

3fcbe6d

Remove unnecessary pyproj<2.0 check

5fe8f06

Merge branch 'main' into refactor-resample

ea17ec0

Fix resampling related invalid sphinx references

bfdb334

djhoese approved these changes May 29, 2025

View reviewed changes

Refactor resampler module to submodules #3124

Are you sure you want to change the base?

Refactor resampler module to submodules #3124

Conversation

pnuu commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

djhoese commented May 12, 2025

Uh oh!

codecov bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coveralls commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15308592336

Details

💛 - Coveralls

Uh oh!

pnuu commented May 13, 2025

Uh oh!

djhoese left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djhoese commented May 13, 2025

Uh oh!

pnuu commented May 16, 2025

Uh oh!

djhoese left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djhoese left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pnuu commented May 12, 2025 •

edited

Loading

codecov bot commented May 13, 2025 •

edited

Loading

coveralls commented May 13, 2025 •

edited

Loading