Skip to content

Refactor resampler module to submodules #3124

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented May 12, 2025

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

@pnuu pnuu self-assigned this May 12, 2025
@pnuu pnuu requested review from djhoese and mraspaud as code owners May 12, 2025 14:45
@pnuu pnuu added component:resampling cleanup Code cleanup but otherwise no change in functionality labels May 12, 2025
@djhoese
Copy link
Member

djhoese commented May 12, 2025

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).

Copy link

codecov bot commented May 13, 2025

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

coveralls commented May 13, 2025

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 Coverage Status
Change from base Build 15307805052: -0.01%
Covered Lines: 55887
Relevant Lines: 58013

💛 - Coveralls

@pnuu pnuu moved this to In Progress in PCW Spring 2025 May 13, 2025
@pnuu
Copy link
Member Author

pnuu commented May 13, 2025

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

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

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?

@djhoese
Copy link
Member

djhoese commented May 13, 2025

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 pnuu moved this from In Progress to Ready for review in PCW Spring 2025 May 14, 2025
@pnuu
Copy link
Member Author

pnuu commented May 16, 2025

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.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

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?

if paths:
return paths
else:
return get_config_path("areas.def")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +92 to +96
try:
from .native import get_native_resampler_classes
resamplers.update(get_native_resampler_classes())
except ImportError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Comment on lines +6 to +11
def get_ewa_resampler_classes():
"""Get bucket resampler classes."""
return {
"ewa": DaskEWAResampler,
"ewa_legacy": LegacyDaskEWAResampler,
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup but otherwise no change in functionality component:resampling
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

4 participants