Skip to content
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

Support multiple readers in group_files and MultiScene.from_files #1269

Merged
merged 21 commits into from Jul 29, 2020

Conversation

gerritholl
Copy link
Collaborator

@gerritholl gerritholl commented Jul 21, 2020

Support multiple readers in group_files and MultiScene.from_files

Adapt group_files so that group_files and MultiScene.from_files can support multiple readers.

Added a test for using multiple readers in a MultiScene.  Note that
the corresponding functionality is not yet implemented and actually
not even yet properly defined (see pytroll#1268).  To implement this in
`MultiScene.from_files` requires updating `satpy.readers.group_files`.

This commit does change the implementation of the keyword-only argument in
`satpy.readers.group_files` by using Pythons own keyword-only argument
functionality (available since Python 3.0).  This has the effect that
failing to pass the `reader` to that function now raises a TypeError
rather than previously a ValueError.
Refactor readers.group_files as a preparation for allowing multiple
readers.  Some functionality is now relegated to dedicated functions.
Single-reader now works again (at least tests pass!), multiple readers
are raising a proper NotImplementedError rather than silently doing
weird stuff; but in upcoming commits I intend to make multiple readers
work, matching non-primary sensors to the primary using the provided
tolerance, dropping the other non-primary sensors with a warning or
error (configurable).
Add support for multiple readers in group_files.  It appears to work,
except that I have broken some other unit tests that I will investigate
in the next commits.
@ghost
Copy link

ghost commented Jul 22, 2020

Congratulations 🎉. DeepCode analyzed your code in 0.879 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Fix the usage of custom grouping keys.
For the new multi-sensor behaviour in group_files, improve test coverage
by adding more cases.  Also point out that when passing multiple
readers, it is strongly recommended to pass group_keys.
@gerritholl gerritholl changed the title WIP: support multiple readers in MultiScene Support multiple readers in MultiScene Jul 22, 2020
@gerritholl gerritholl changed the title Support multiple readers in MultiScene Support multiple readers in group_files and MultiScene.from_files Jul 22, 2020
In MultiScene.from_files, for the use case when passing multiple
readers, add a flag to return only those groups where all readers have
at least one file found.  The default remains that it returns those
groups where at least one reader has at least one file found.
When no reader is passed to group_files and some readers can't be
imported, skip those while logging the exception communicating the
unimportability.
@gerritholl gerritholl marked this pull request as ready for review July 22, 2020 16:12
Under protest, but aligning strings to the beginning of the first line's
string prefix rather than with the opening quote thereof.
@gerritholl
Copy link
Collaborator Author

(and again, codebeat thinks I've been deleting several files)

@coveralls
Copy link

coveralls commented Jul 22, 2020

Coverage Status

Coverage increased (+0.02%) to 90.079% when pulling ae0eb04 on gerritholl:multiscene-multireader into 2b73246 on pytroll:master.

Add an example to the MultiScene documentation on how to combine
multiple readers into a single MultiScene.
There was a missing continue statement to accompany the message that
there was a YAMLConstructorError.
@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #1269 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   90.06%   90.07%   +0.01%     
==========================================
  Files         218      218              
  Lines       31530    31580      +50     
==========================================
+ Hits        28397    28447      +50     
  Misses       3133     3133              
Impacted Files Coverage Δ
satpy/multiscene.py 90.06% <100.00%> (+0.06%) ⬆️
satpy/readers/__init__.py 95.44% <100.00%> (+0.31%) ⬆️
satpy/tests/test_multiscene.py 99.42% <100.00%> (+0.01%) ⬆️
satpy/tests/test_readers.py 99.03% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b73246...ae0eb04. Read the comment docs.

Documentation clarification on the MultiScene.from_files construction.
It was of course already possible to have multiple readers in a
MultiScene, just it couldn't be automatically constructed with
from_files.
In the new part of the MultiScene documentation, fix sphinx syntax as
well as a typo in a method name.
In the new MultiScene documentation, fix the cross-reference type to the
satpy.Scene class and clarify a bit more on the example with the
superimposed lightning.
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM, I just have one language comment.

satpy/multiscene.py Show resolved Hide resolved
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.

Really nice job. Thanks for taking the time to get this implemented and all of the docstrings and type hints. I had a couple suggestions and possible issues, but otherwise it looks really good and is really close to merging.

doc/source/multiscene.rst Outdated Show resolved Hide resolved
doc/source/multiscene.rst Outdated Show resolved Hide resolved
doc/source/multiscene.rst Outdated Show resolved Hide resolved
satpy/readers/__init__.py Outdated Show resolved Hide resolved
satpy/tests/test_readers.py Outdated Show resolved Hide resolved
gerritholl and others added 5 commits July 28, 2020 17:30
For the multiscene-multifile routine, improve the documentation, change
a variable name, and raise a warning if no matching keys are found for
grouping, which may happen if the user passes a string rather than a
collection of strings.
Added a real non-breaking space in the file source via the Github editor, avoiding the use of a sphinx declaration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:multiscene component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple readers in MultiScene.from_files
4 participants