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

Adding AdvancedMaskingAdapter and test #190

Merged
merged 1 commit into from Jun 24, 2020
Merged

Conversation

awst-baum
Copy link
Collaborator

This PR adds an AdvancedMaskingAdapter that can be used to mask the data from a reader based on a list of filters. The list must contain 3-tuples of filter parameter: data column to filter on, operator to use and threshold to use. Operator can either be taken from a pre-defined list (same as in MaskingAdapter) or be a user-defined callable.
We'd like to use it in QA4SM to implement complex masking for SMOS data and think that it'll be useful in pytesmo in general.

Example of SMOS filter:

filtered_reader = AdvancedMaskingAdapter(reader, [
    ('Scene_Flags', smos_exclude_bitmask, 0b00001000), 
    ('Soil_Temperature_Level1', '>=', 273), ])        

(where smos_exclude_bitmask is a user-defined function)

Copy link
Member

@wpreimes wpreimes left a comment

Choose a reason for hiding this comment

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

I think that's a good way to handle bitflags, @tracyscanlon is also is/was also working on something similar I think, maybe you can give some feedback wrt. C3S/CCI flags as well?

@@ -160,6 +160,68 @@ def read(self, *args, **kwargs):
data = super(SelfMaskingAdapter, self).read(*args, **kwargs)
return self.__mask(data)


class AdvancedMaskingAdapter(BasicAdapter):
Copy link
Member

@wpreimes wpreimes Jun 8, 2020

Choose a reason for hiding this comment

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

This is just a strictly more flexible implementation for a SelfMaskingAdapter right? Would it make sense to inherit SelfMaskingAdapter from this class to show that it basically does the same but with less options? It just might confuse people which adapter they should use if there are many similar options (many people use pytesmo just as a toolbox)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It fills the same evolutionale niche as the SelfMaskingAdapter, yeah. Personally, I wouldn't change the SelfMaskingAdapter at all (since I only left it in for compatability). Well, maybe change it to add a deprecation warning - and then remove it some time in the future.
If we add a deprecation warning and a comment a la 'please use AdvancedMaskingAdapter instead', this should help users decide what to use.

@wpreimes wpreimes merged commit ca49902 into master Jun 24, 2020
@wpreimes wpreimes deleted the advanced_masking_adapter branch July 2, 2020 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants