Add masking helpers to validation framework #140
Conversation
| threshold: | ||
| value to use as the threshold combined with the operator | ||
| column_name: string | ||
| name of the column to cut the read masking dataset to |
There was a problem hiding this comment.
I guess this should be something like name of the column to apply the threshold to?
There was a problem hiding this comment.
Yes, that sounds better (although the given column will be the only one remaining in the DataFrame after masking).
There was a problem hiding this comment.
Really? How I read the code the complete dataframe will be returned but masked by the boolean mask
There was a problem hiding this comment.
Are we talking about the changes in MaskingAdapter or about the SelfMaskingAdapter?
There was a problem hiding this comment.
SelfMaskingAdapter. Line 101 of the diff.
There was a problem hiding this comment.
Oh, sorry, my bad. I've changed it.
| return self.operator(data, self.threshold) | ||
| return self.__mask(data) | ||
|
|
||
| class SelfMaskingAdapter(object): |
There was a problem hiding this comment.
Could you add a small paragraph to the documentation about the SelfMaskingAdapter and the differences to the MaskingAdapter. I think that would help us in making it clearer what they both do. I think we might be able to combine them into one class since they are very similar. What do you think?
There was a problem hiding this comment.
Re documentation, sure, as soon as we've sorted out what we actually want to do. Re combining: pro: avoiding code repetition, contra: both adapters do something different and they yield a quite different result. My personal taste as a dev would be to have two separate classes for two separate concerns - but I could derive them from a common base class to avoid code repetition. If I do that, should I use an https://docs.python.org/2/library/abc.html ?
There was a problem hiding this comment.
For the documentation I think we should edit the ipython notebooks and automatically include them in the sphinx documentation. For now editing https://github.com/TUW-GEO/pytesmo/blob/master/docs/validation_framework.ipynb and exporting it as rst should also work.
There was a problem hiding this comment.
I think we can keep the classes separate for now.
There was a problem hiding this comment.
OK, I've updated the docs - what do you think?
There was a problem hiding this comment.
Looks good, I'll merge this after a quick rebase if all the tests still pass.
…ng dataset to one specific column
…version from fixed examples
Extends the existing MaskingAdapter so that it can be used reduce multi-column masking datasets to one column and do "not equal" comparisons. Adds a SelfMaskingAdapter that wraps a data reader and allows to mask the data while reading, based on a comparison operation on one column (column, operation, and threshold can be specified).