-
Notifications
You must be signed in to change notification settings - Fork 24
test coverage for filters #229
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
test coverage for filters #229
Conversation
| raise TypeError("This filter only accepts xr.DataArray or xr.Dataset") | ||
|
|
||
| if not function(sample): | ||
| if not drop: |
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.
From the doc string for DropValue:
Filter to drop data containing more than a given percentage of a value.
all the DropValue filters calculate whether % of elemns matching value >= the percentage, and then not's them here, which does the opposite of what the doc string says. @tennlee should I follow what the doc string says or leave the logic as is?
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 top-level filter class says "filter should sairse a PipelineFilterException if invalid. So I think the not is unwanted? The test for me is that a pipelinefilterexception should be raised if data with a high proportion of nans (or whatever query value) is supplied.
Pull Request Test Coverage Report for Build 20294261772Details
💛 - Coveralls |
152094d to
0b43a9f
Compare
|
covered dask case so pr is ready to review. |
|
not sure why ci has suddenly started failing at install depends step... |
|
Sorry for the delay here ... a few things at work meant a building of reviews, and I had some difficult refactoring to do on "scores". I'll get to this next week, maybe earlier. |
|
I note the unit tests are failing due to the dependencies now exceeding the disk space allowed by the CI/CD system. I am trying to push up changes to bring the CI/CD setup back within the tolerances set by Github. |
|
@edoyango okay, so I think the CI/CD is doing its job again. However, surprisingly, one of the tests i failing on Python 3.13 specifically, which is unexpectedly. Let me know if you can take a look. If I can figure it out, I'll happily push up a fix, but the answer is not springing to mind. |
|
@edoyango I'm going to merge this to get the obvious benefits from improved coverage and getting the CI/CD passing again. However I've raised a new issue around the point you identified about the directionality of the logic in the filter. If you have time to address it, please do. |
This adds test coverage for filtering for numpy, dask, xarray. Contains a few tests unrelated to filtering. Some of the xarray filtering seemed improperly implemented (looks like it was copied from numpy), so I added some proper implementations.
WIP.