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

Fix negative axes reductions. #118

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

hameerabbasi
Copy link
Collaborator

Fixes #117

if set(axis) == set(range(self.ndim)):
result = method.reduce(self.data, **kwargs)
if self.nnz != self.size:
result = method(result, _zero_of_dtype(self.dtype)[()], **kwargs)
else:
axis = tuple(axis)
neg_axis = tuple(ax for ax in range(self.ndim) if ax not in axis)
neg_axis = tuple(ax for ax in range(self.ndim) if ax not in set(axis))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? I'm not entirely sure of Python semantics here but I suspect that you're creating a new set every time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes it O(ndim) rather than O(ndim^2). I ran a quick test to see if this was indeed the case:

>>> class A:
...     def get_set(self):
...         print("Called")
...         return set(range(5))
...     
>>> tuple(i for i in A().get_set())
Called
(0, 1, 2, 3, 4)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really matter either way. Asymptotic arguments don't really carry significance when n is likely to be very small.

In [1]: axis = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)

In [2]: ndim = 20

In [3]: %timeit tuple(ax for ax in range(ndim) if ax not in axis)
The slowest run took 6.44 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 4.03 µs per loop

Happy either way though. I'm glad to learn that Python handles things in that order.

@@ -712,13 +712,15 @@ def reduce(self, method, axis=(0,), keepdims=False, **kwargs):
if not isinstance(axis, tuple):
axis = (axis,)

axis = tuple(a if a >= 0 else a + self.ndim for a in axis)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other uses of axis= where this should be done but is not yet currently done?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a short "find all" of axis, I'm inclined to say no. It does appear in tensordot, concatenate and stack but those already have this.

@codecov-io
Copy link

codecov-io commented Mar 2, 2018

Codecov Report

Merging #118 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   96.04%   96.04%   +<.01%     
==========================================
  Files          11       11              
  Lines        1896     1897       +1     
==========================================
+ Hits         1821     1822       +1     
  Misses         75       75
Flag Coverage Δ
#python27 95.15% <100%> (ø) ⬆️
#python36 96.04% <100%> (ø) ⬆️
Impacted Files Coverage Δ
sparse/coo.py 95.05% <100%> (ø) ⬆️
sparse/tests/test_coo.py 99.83% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97d5690...2e470ae. Read the comment docs.

@hameerabbasi hameerabbasi merged commit 217ca23 into pydata:master Mar 2, 2018
@hameerabbasi hameerabbasi deleted the fix-reduce-negative-axis branch March 3, 2018 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants