-
Notifications
You must be signed in to change notification settings - Fork 24
Add tests for select ops and update numpy+dask Select class #233
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
base: develop
Are you sure you want to change the base?
Conversation
this fixes the implementation of numpy.Select so that the results match what is stated in the doc string. This changes the implementation to use indexing with native slices.
this fixes the implementation of dask.Select so that the results match what is stated in the doc string. This changes the implementation to use indexing with native slices.
Pull Request Test Coverage Report for Build 20802458553Details
💛 - Coveralls |
|
I'm also having trouble parsing what was done previously. As long as:
then the changes seem fine. I'm not familiar enough with the codebase but |
The new functionality ensures numpy and dask select match the doc string example. The description, however, is probably outdated as select doc string seems to be select array sections using a given index list - rather than an element.
It's a little hard to say that all the backends have consistent behaviour, as xarray doesn't have a select class - instead it has
I couldn't find any tutorials that use the Select operation, so there shouldn't be any regressions. |
|
Hmm, well dask and numpy definitely need to be consistent since they are pretty fundemental APIs. With xarray it actually uses dask or numpy under the hood so I’m not sure why it’s a bit different. We can revisit it later I guess. For now at least the logic is a bit more readable. odd that there’s no tutorials using select, given it’s pretty much bread and butter for dissecting data. I guess they all use the entire dataset all the time? Or maybe I’m misunderstanding what select does and it’s something more internal. I’m also not sure what the difference between select and slice would be. Again I’m used to “slice of indices” to “select data”, so a slice is essentially a map to a particular selection, I haven’t looked into Slice in the context of PET to comment further. In any case I’m fine with the changes given there’s no regression. We can revisit xarray backend being a bit different later. |
In addition to adding tests, this "fixes" the numpy and dask Select classes so that they do what is said in the doc string. For example,
prints
instead of the expected (as per the doc string)
the old numpy Select does the same thing.
I found the logic difficult to follow, so to correct the behaviour, I rewrote the classes to use numpy/dask's native tuple/slice indexing instead.
The xarray select class does the right thing so didn't need changes.