Find pdist with known shape
#71
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakirkham
commented
Oct 6, 2017
| result = _compat._ravel(result)[_compat._ravel(mask)] | ||
| result = dask.array.concatenate([ | ||
| result[i, i + 1:] for i in range(0, len(result) - 1) | ||
| ]) |
There was a problem hiding this comment.
Am a little concerned about this performance-wise for large numbers of points. Reason being this makes the graph balloon with getitem entries. Would be good if we could cut this down somehow, but it is not obvious to me how without reusing the old masking strategy.
There was a problem hiding this comment.
That said, it seems to do ok given reasonable chunk sizes when playing around with it locally. So perhaps this is not worth worrying about until use cases that have issues present themselves.
jakirkham
force-pushed
the
pdist_knwn_shape
branch
2 times, most recently
from
c5aa4ac
to
c8e510f
Compare
Instead of using `triu` and masking out the upper triangle from the flattened array, simply slice out portions corresponding to the upper triangle and use `concatenate` to join them all together into the result. This confers a number of benefits. First that the shape of the result is known instead of being unknown. Second no zero arrays are generated in the process. Third compatibility functions related to reshaping and generating indices can be dropped reducing the maintenance overhead. As far as optimizations go, this should be similar to `triu` as it results in dropping out the same chunks that `triu` would have. For the chunks that are dropped out completely, this strategy will be more performant than `triu` as it will not generate zeros in there place. For the chunks along the diagonal, this should have similar performance as there will be some duplication of effort for these computations. However slicing out the selections of interest should be a little more performant than calling NumPy's `triu` due to using less memory and potentially avoiding a copy.
As we are no longer using `_ravel` in `pdist`, we no longer need to keep our own internal implementation of `_ravel`. So drop the function itself and associated tests. Should lighten our maintenance burden.
As we are no longer using `_indices` in `pdist`, we no longer need to keep our own internal implementation of `_indices`. So drop the function itself and associated tests. Should lighten our maintenance burden.
Make sure that before computing `pdist` (while we simply have a Dask Array) the shape is known and matches the expected shape of an equivalent computation from NumPy.
jakirkham
force-pushed
the
pdist_knwn_shape
branch
from
c8e510f
to
630d1c1
Compare
jakirkham
commented
Oct 8, 2017
|
|
||
| result = _compat._ravel(result)[_compat._ravel(mask)] | ||
| result = dask.array.concatenate([ | ||
| result[i, i + 1:] for i in range(0, len(result) - 1) |
There was a problem hiding this comment.
Missed using irange here. Fixing in PR ( #80 ).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of using
triuand masking out the upper triangle from the flattened array, simply slice out portions corresponding to the upper triangle and useconcatenateto join them all together into the result.This confers a number of benefits. First that the shape of the result is known instead of being unknown. Second no zero arrays are generated in the process. Third compatibility functions related to reshaping and generating indices can be dropped reducing the maintenance overhead.
As far as optimizations go, this should be similar to
triuas it results in dropping out the same chunks thattriuwould have. For the chunks that are dropped out completely, this strategy will be more performant thantriuas it will not generate zeros in there place. For the chunks along the diagonal, this should have similar performance as there will be some duplication of effort for these computations. However slicing out the selections of interest should be a little more performant than calling NumPy'striudue to using less memory and potentially avoiding a copy.