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.
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
Add option to replace saturated MODIS L1b values with max valid value #2057
Add option to replace saturated MODIS L1b values with max valid value #2057
Changes from 9 commits
c642b3a
b68fe96
fb5cfb4
e93ba67
d25145f
3742cff
4a0bea6
62c2a1c
445ce79
17c26db
856fc9e
d5a8f62
ca709b7
5e8a088
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What are the kwargs for?
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.
The kwargs are actually needed if this reader (file handler) is loaded at the same time as other readers. This is an unfortunate side effect of how the Scene handles reader keyword arguments and we've done it in other readers. The Scene, when given a dictionary of keyword arguments, will pass them to all readers being loaded. If a file handler/reader doesn't have
**kwargs
then any unrecognized keyword arguments will raise an exception.Recently @gerritholl added the ability to specify the exact reader you want to pass keyword arguments to from the Scene
__init__
so maybe this isn't required anymore, but I'd be nervous about deprecating it too fast. I could remove it for this reader with the understanding that I only need to handlemask_saturated
and the assumption that the base HDFEOS file handler has no other keyword arguments. I could then remove the**kwargs
that I added to the base HDFEOS file handler above.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.
So I looked at this more and I think we need this in the other file handlers. The base and geo file handlers in
hdfeos.py
will also both receive themask_saturated=True
keyword argument and won't know what to do with it. The easiest way to capture that and ignore it is to use**kwargs
.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.
I don't really like having one function doing two things here, could we move the
if self._mask_saturated
logic here and split that function?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.
I could, but it would result in duplicated code as the two options both check
valid_min
...nevermind, re-reading it is clear that I could do the masking first as a separate method. I'll see what I can do.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.
I separated this out. I'm not sure if one placement of the
if
statement is better than another. Let me know what you think.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.
This looks like a lot of return values :) Would it be worth having a class for this?
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.
or build the DataArray here and put eg
uncertainty[band_index]
as an ancillary dataset in the attrs?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.
Yeah I didn't like this much either, but it wasn't obvious how to do it any other way. It overall seemed better than the huge method that was here before. My goal was to avoid letting
datasets
ordataset
(the name of the variable in the file to use) leak to the outer scope, but maybe I make one method that returns dataset and band_index...nah because then I still have toself.sd.select
the dataset again and get the var_attrs.The problem with making the DataArray here is that it actually doesn't get me much since var_attrs isn't actually applied to the DataArray at all. I'm not sure how I feel about the uncertainty indexes going in the attrs either. I'm leaning towards don't like it 😉
I'll think about doing a separate helper class or something while I work on other stuff today.
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.
Ok so it required a tiny bit of duplicated logic (
.select
), BUT I think it is overall cleaner. Check it out.