Refactor satpy.writers to separate modules #3122
Conversation
djhoese
added
the
backwards-incompatibility
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3122 +/- ##
=======================================
Coverage 96.23% 96.24%
=======================================
Files 398 400 +2
Lines 57325 57373 +48
=======================================
+ Hits 55168 55218 +50
+ Misses 2157 2155 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 14935895156Details
💛 - Coveralls |
| def _determine_mode(dataset): | ||
| if "mode" in dataset.attrs: | ||
| return dataset.attrs["mode"] | ||
|
|
||
| if dataset.ndim == 2: | ||
| return "L" | ||
| if dataset.shape[0] == 2: | ||
| return "LA" | ||
| if dataset.shape[0] == 3: | ||
| return "RGB" | ||
| if dataset.shape[0] == 4: | ||
| return "RGBA" | ||
| raise RuntimeError("Can't determine 'mode' of dataset: %s" % | ||
| str(dataset)) |
There was a problem hiding this comment.
FYI this function, as far as I can tell, isn't used anywhere so I deleted it (I did not move it to a submodule).
There was a problem hiding this comment.
LGTM!
The only little thing I found, was that fallback import stuff in satpy.writers.__init__.py are not tested, so "we are not sure" they work. There could also be a deprecation warning or something o tell the users not to import directly from the writer subpackage.
|
You're right I should add tests for the import deprecation and add a specific sentence about how imports should be done now. As mentioned on slack the naming is tricky. Regarding |
|
Looks good to me too! Moving the |
Yes @ameraner, I agree. That's why I'm really concerned about getting the submodules and what goes in those submodules right before merging this. Like maybe |
| from satpy.writers.utils import get_enhanced_image | ||
|
|
There was a problem hiding this comment.
Two questions/comments on function-level imports:
- Can we have some guidance on what imports shall be avoided on a module level? I'm confused why this would apply to an import from within satpy.
- Does this need to be tested? I can imagine someone (such as me) might easily re-introduce an import trollimage on a module level somewhere, and I don't think any test would warn against this.
There was a problem hiding this comment.
You're absolutely right. This should probably be guarded against on the callee side (inside get_enhanced_image). For now I went with the simple rule of "if it is only used by a few things inside the module then import it in-function". What I think we could do, especially after Tuesday's meeting this week, is actually remove trollimage as a hard requirement and have a test environment that runs "minimal" tests in a "minimal" environment.
The biggest problem with this particular change that you commented on is that satpy/composites/__init__.py is waaaaaay too big and needs to be split up by purpose. Similar to this PR there should be warnings on import and no real code in the __init__.py. Then it wouldn't matter much if you put an import like get_enhanced_image or zarr or even some other third-party package at the module level as that sub-module should only be imported when used.
Avoids unnecessary imports
|
I'm realizing after thinking about the "base_image.py" -> "image.py" suggestion that was either here or on slack, the "satpy/writers/base.py" could be just "satpy/writers/writer.py". That might be nice in that it keeps "base" and "core" out of the module naming, but also might make it difficult for new users/contributors to find the base classes. "satpy/writers/core/writer.py"? Maybe that's going too far. |
This is my attempt at making Satpy's use of dependencies more flexible for users (me) that don't need all of the usual functionality of Satpy (readers -> resampling -> compositing -> writing) and therefore don't need all of satpy's usual dependencies.
The primary changes here are:
satpy.writersto make it sotrollimagedoesn't get imported unless it is actually used and that we don't actually import things that aren't used. This saves anXRImageimport but also arasteriobeing installed as a dependency if trollimage is not installed (conda-forge's trollimage includes rasterio by default I think).flatten_dictfromsatpy.writers.utilstosatpy.utilssince it was being used by thesatpy/dataset/metadata.pymodule. This means that writers don't have to be updated in the other parts of satpy that combine metadata (e.g. readers and composites).zarrimport in the resample module sozarrisn't needed if you aren't using zarr-based caching.Reviewers: Please be extra critical of the new module names in
satpy.writersas I would like to have user import things from these module names rather thansatpy.writersdirectly. This does mean users would be expected to dofrom satpy.writers.utils import get_enhanced_image. Also pay attention to if things insatpy.writers.utilsshould be somewhere else.AUTHORS.mdif not there already