Skip to content

MAINT: Fix all special-casing of dtypes in count_nonzero #9849

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

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Oct 11, 2017

This is a redo of #7177

Rationale:

  • int.astype(bool) is marginally faster than int == 0 (profiled as ~5%)
  • bool.astype(bool, copy=False) should be comparable to a python if
  • apply_along_axis is slow, and probably loses whatever we gain by avoiding sum

I haven't rerun @gfyoung's benchmark suite that came with #7177

This also won't pass until #9848 #9856 is merged

@mhvk
Copy link
Contributor

mhvk commented Oct 11, 2017

Looks good to me, if only for the increase in clarity.

@shoyer
Copy link
Member

shoyer commented Oct 11, 2017

Looks like this fails for some dtypes. From Travis:

======================================================================
ERROR: numpy.core.tests.test_numeric.TestNonzero.test_count_nonzero_axis_all_dtypes
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/numpy/core/tests/test_numeric.py", line 1070, in test_count_nonzero_axis_all_dtypes
    assert_equal_w_dt(np.count_nonzero(m, axis=0),
  File "/home/travis/build/numpy/numpy/builds/venv/lib/python3.6/site-packages/numpy/core/numeric.py", line 413, in count_nonzero
    return a.astype(np.bool_, copy=False).sum(axis=axis, dtype=np.intp)
ValueError: invalid literal for int() with base 10: ''

@eric-wieser eric-wieser force-pushed the cleanup-count_nonzero branch from 0433d5b to c94e2c5 Compare October 12, 2017 04:47
@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 12, 2017

@shoyer: That's because there's a bugfix this needed from #9848, as I mention in the description.

However, @ahaldane argued the bugfix needed a deprecation cycle, so I've redone this with a workaround.

This still requires #9856 to pass.

A quick profile reveals that `int.astype(bool)` is faster than `int == 0` anyway
@eric-wieser
Copy link
Member Author

That should hopefully trigger a retest now that #9856 is in

@eric-wieser
Copy link
Member Author

@shoyer Tests now pass

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 15, 2017

I'm struggling to get asv to provide useful comparison benchmarks on my machine, frustratingly.

Two consecutive runs give wildly (~50%) different results on the same commit, so comparisons are meaningless. Even more concerningly, some benchmarks complete instantaneously, and it's a different set each time.

I reckon we just merge this, and see what the online asv has to say

@charris charris merged commit 36e716a into numpy:master Oct 16, 2017
@charris
Copy link
Member

charris commented Oct 16, 2017

Thanks Eric.

@eric-wieser
Copy link
Member Author

Massive improvement for object arrays: https://pv.github.io/numpy-bench/#bench_core.CountNonzero.time_count_nonzero_multi_axis.

Maybe the special casing of bool was worth having, but the user would do better to call sum directly in those cases anyway.

@eric-wieser
Copy link
Member Author

eric-wieser commented Nov 17, 2017

Although actually, a performance hit for very large object arrays, presumably because we now allocate a large boolean array too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants