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

ENH: implement any and all functions #175

Merged
merged 4 commits into from Aug 5, 2018

Conversation

stsievert
Copy link
Contributor

This PR closes #173 and adds any and all methods to COO.

Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

Great effort. I've left a few comments. Overall, this looks great, just needs some tweaks, namely in the tests and docs.

docs/generated/sparse.COO.rst needs the function names added in order to generate the docs.

You will need to generate the docs once in order to produce docs/generated/sparse.COO.all.rst and docs/generated/sparse.COO.any.rst and add those to git. See the section on adding docs in the CONTRIBUTING.

@@ -811,6 +811,127 @@ def max(self, axis=None, keepdims=False, out=None):
"""
return np.maximum.reduce(self, out=out, axis=axis, keepdims=keepdims)

def any(self, axis=None, keepdims=False, out=None):
"""
Minimize along the given axes. Uses all axes by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to be amended.

--------
:obj:`numpy.min` : Equivalent numpy function.
scipy.sparse.coo_matrix.min : Equivalent Scipy function.
:obj:`nanmin` : Function with ``NaN`` skipping.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The See also section needs amending.


>>> x = np.add.outer(np.arange(5), np.arange(5))
>>> x # doctest: +NORMALIZE_WHITESPACE
array([[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.

Should ideally be a boolean array that's sometimes false and sometimes true.

See Also
--------
:obj:`numpy.all` : Equivalent numpy function.
scipy.sparse.coo_matrix.all : Equivalent Scipy function.
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 exist, I'd cite any and all from each other.


>>> x = np.all.outer(np.arange(5), np.arange(5))
>>> x # doctest: +NORMALIZE_WHITESPACE
array([[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.

Same here. Ideally use this array for both to show the output, it's like a truth table:

x = np.array([
    [False, False],
    [False, True],
    [True, False],
    [False, False]
])

(It could be formatted better)

@@ -16,6 +16,8 @@
('sum', {'dtype': np.float16}, {'atol': 1e-2}),
('prod', {}, {}),
('min', {}, {}),
('all', {}, {}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need separate tests, input should be np.bool_ arrays. Take a look at how we've produced random arrays for binary ops and use that.

@hameerabbasi
Copy link
Collaborator

Oh. Apparently you're using an old NumPy version... The docstrings need to be produced with newer versions to pass the tests, unfortunately. (1.14.0 or newer I think).

@hameerabbasi
Copy link
Collaborator

@stsievert Are you still interested in working on this? If not, is it okay if I pushed to your branch? You'd still be listed as the author of the commit.

@stsievert
Copy link
Contributor Author

is it okay if I pushed to your branch?

That's fine by me.

@codecov
Copy link

codecov bot commented Aug 5, 2018

Codecov Report

Merging #175 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   97.14%   97.22%   +0.07%     
==========================================
  Files          11       11              
  Lines        1259     1295      +36     
==========================================
+ Hits         1223     1259      +36     
  Misses         36       36
Impacted Files Coverage Δ
sparse/coo/core.py 96.73% <100%> (+0.03%) ⬆️
sparse/slicing.py 99.2% <0%> (ø) ⬆️
sparse/coo/indexing.py 99.18% <0%> (+0.27%) ⬆️

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 6cb64a5...727e6e5. Read the comment docs.

@hameerabbasi
Copy link
Collaborator

It seems like NumPy 1.15 changed the docstrings for bool arrays, omitting the dtype. For the tests to pass both locally and in Travis, I needed to skip these doctests.

@hameerabbasi hameerabbasi merged commit 968155c into pydata:master Aug 5, 2018
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.

Add any and all methods.
2 participants