-
Notifications
You must be signed in to change notification settings - Fork 298
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
Modis l2 available datasets #913
Open
BENR0
wants to merge
15
commits into
pytroll:main
Choose a base branch
from
BENR0:modis_l2_available_datasets
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+147
−40
Open
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
9173def
Add available datasets method template to modis_l2 reader
BENR0 7ef3452
Add add_offset to hdfeos dataset reading
BENR0 7f1c572
Add available_datasets method to modis_l2 reader
BENR0 cb2272b
Change resolution dict to use only columns
BENR0 f36993c
Merge: main
BENR0 8a73937
Merge branch 'main' into modis_l2_available_datasets
9d43755
fix: dynamic datasets overwriting configured datasets.
d7eb4d2
doc: update docstrings and comments.
60e4d21
Merge branch 'main' into modis_l2_available_datasets
34dc47f
Merge branch 'main' into modis_l2_available_datasets
8438837
Update satpy/readers/modis_l2.py
mraspaud 0cc050f
test: add tests.
afe779f
doc: update documentation.
ee32f84
Merge branch 'main' into modis_l2_available_datasets
bb5f538
merge main
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem with this is that it overlaps with the mod35_hdf pattern so a file could be matched to either file type depending on which one was checked first. Either we have a file pattern for each possible level 2 filename or one single generic one. There are two problems with the single generic one:
cloud_mask
specially in available_datasets so that it is only shown as available for the mod35 file. Not a huge deal, but could be confusing to some.In the future we could make the base reader check the file handler after it is created to see if it changed it's file type. For example the ModisL2HDFFileHandler could say "I see that 'product' in the filename is XX so my file type is actually 'modis_l2_product_XX'". The base reader then knows how to organize the files. @mraspaud did you have to do anything like this for the VIIRS SDR reader when you updated it for the various file naming schemes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I guessed that this overlap might pose a problem. But I after experimenting with the order of the patterns I thought that the reader took the first match which would work in the current situation but might be difficult in the future if more datasets are added to the yaml file. I think it would be nice if the reader could also identify if any dataset in the file needs special treatment like bit decoding (then there would be no need for specifying additional datasets in the yaml file) . But this might not be easy, if doable at all.
The problem with different level 2 product files given to the reader at the same time also occurred to me but I thought that most users might be smart enough to keep product files in different directories but I guess that is a hopeful wish and sooner or later somebody will discover this "bug". I think I don't understand enough of the inner workings of how the readers are initialized (interested though to get a better understanding of what is done when and the idea behind it) to judge what would be the best way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I'm 99% sure that there is no guarantee about the order of which file pattern/file type is checked first so it might come down to luck. Maybe let's continue the file type discussion on slack to nail down what I'm thinking. I could make a separate PR for the functionality I'm thinking of and then we can incorporate it in to this PR after it is merged.