reverse logic for DropValue filters #232
Conversation
This aligns the logic with the doc strings. Now a PipelineFilterException is raised when the number of values in the input is equal to or greater than the supplied percentage threshold. Before, the exception would be raised if the number of values is strictly less than the supplied threshold.
Pull Request Test Coverage Report for Build 20800831494Details
💛 - Coveralls |
| Sample to check | ||
| Returns: | ||
| (bool): | ||
| If sample contains nan's |
There was a problem hiding this comment.
probably not to do with this PR specifically, but this description implies it's returning a bool.
But the implementation itself raises an exception... (or returns None), so I'm a bit confused on how this works at a high level.
If it could be modified to be made clearer, that would help I think.
There was a problem hiding this comment.
I agree it is confusing - prior to any of my changes the implementation was raising an exception and never returned a logical. Perhaps there was a code change that wasn't reflected in the doc string?
In any case, happy to update the doc strings in a separate pull request.
There was a problem hiding this comment.
Yea that would be great, thanks.
|
The change itself seems fine, the difficulty I'm having is the flag (bool) according to docstring and whether True => exception or False => exception, and that's more my lack of understanding probably. |
3 participants
This aligns the logic with the doc strings and resolves #231. Now a PipelineFilterException is raised when the number of values in the input sample is equal to or greater than the supplied percentage threshold. Before, the exception would be raised if the number of values is strictly less than the supplied threshold.