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

Use mask selection in compress #4548

Merged
merged 7 commits into from Mar 6, 2019

Conversation

Projects
None yet
3 participants
@jakirkham
Copy link
Member

commented Mar 5, 2019

Fixes #4497

Rewrites compress to leverage mask selection in its implementation. This simplifies the code a bit as it benefits from existing mask selection support for Dask Arrays ( #2658 ), which works just as well for NumPy arrays or other similar arrays.

cc @pentschev @mrocklin

  • Tests added / passed
  • Passes flake8 dask

jakirkham added some commits Mar 5, 2019

Trim Dask Array that extends past condition
If the `condition` is shorter than the Dask Array, treat it as if it is
filled with `False` values at the end. In other words, trim the Dask
Array down to the length of the `condition`. This works just as well
when the `condition` is a Dask Array as when it is a NumPy array.
Use masked selection to implement compress
As masked selection is supported by Dask Arrays, NumPy arrays, and other
array types, go ahead and change the `compress` code to use masked
selection in all cases. Should make it easier for other array types to
benefit from this code. Simplifies our code and removes a branch as
well.

@jakirkham jakirkham force-pushed the jakirkham:use_mask_compress branch from c796322 to 1e9f445 Mar 6, 2019

jakirkham added some commits Mar 6, 2019

Drop `condition` length check
If the length of `condition` is shorter, the array will be shortened
with or without this check. Otherwise if the length of `condition` is
the same, then this will be a no-op (and we already test for this). In
the rare event `condition` is longer than our array, this will do
nothing, but we will generate an error later.
Simplify slicing with `condition`
Uses the same comprehension trick for truncating the Dask Array above
when slicing with the mask.
@mrocklin

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

This simplifies the code a bit

Indeed :)

Merging tomorrow if there are no further comments

@pentschev

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Oh wow, that was quick, and the code is much cleaner now! Thanks @jakirkham!

@mrocklin mrocklin merged commit 7bf6172 into dask:master Mar 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jakirkham jakirkham deleted the jakirkham:use_mask_compress branch Mar 6, 2019

jorge-pessoa pushed a commit to jorge-pessoa/dask that referenced this pull request May 14, 2019

Use mask selection in compress (dask#4548)
* Trim Dask Array that extends past condition

If the `condition` is shorter than the Dask Array, treat it as if it is
filled with `False` values at the end. In other words, trim the Dask
Array down to the length of the `condition`. This works just as well
when the `condition` is a Dask Array as when it is a NumPy array.

* Use masked selection to implement compress

As masked selection is supported by Dask Arrays, NumPy arrays, and other
array types, go ahead and change the `compress` code to use masked
selection in all cases. Should make it easier for other array types to
benefit from this code. Simplifies our code and removes a branch as
well.

* Coerce `compress` arguments to Dask Arrays

* Drop unneeded check with `condition` in `compress`

* Swap `condition` and `axis` checks in `compress`

* Drop `condition` length check

If the length of `condition` is shorter, the array will be shortened
with or without this check. Otherwise if the length of `condition` is
the same, then this will be a no-op (and we already test for this). In
the rare event `condition` is longer than our array, this will do
nothing, but we will generate an error later.

* Simplify slicing with `condition`

Uses the same comprehension trick for truncating the Dask Array above
when slicing with the mask.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.