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

EHN: add nanmin/nanmax in scipy.sparse #8902

Closed
wants to merge 7 commits into from

Conversation

glemaitre
Copy link
Contributor

NumPy allows for finding np.min and np.max by ignoring NaN values.
Currently, there is no implementation in the sparse module while np.fmin and np.fmax could be used similarly as np.minimum and np.maximum ufunc.

Our use case is in scikit-learn: we have some pre-processing methods for which we would like to discard NaN to compute those extrema.

@rgommers rgommers added enhancement A new feature or improvement scipy.sparse labels Jun 9, 2018
@rgommers
Copy link
Member

rgommers commented Jun 9, 2018

Makes sense to have nanmin/nanmax. I'm just not sure that these should be methods rather than functions (they're functions in numpy).

@glemaitre
Copy link
Contributor Author

Makes sense to have nanmin/nanmax. I'm just not sure that these should be methods rather than functions (they're functions in numpy).

It was one of my concern as well. Would you make them accessible by scipy.sparse.nanmin and scipy.sparse.nanmax.

@rgommers
Copy link
Member

rgommers commented Jun 9, 2018

For symmetry with numpy, that would be my preference I think. Since this is a new feature, it would be useful if you could mention this on the scipy-dev mailing list to see if others have opinions.

@glemaitre
Copy link
Contributor Author

glemaitre commented Jun 9, 2018 via email

@charris
Copy link
Member

charris commented Jun 9, 2018

The current nep for numpy __array_function__ might be relevant here. @shoyer Comment?

@shoyer
Copy link
Contributor

shoyer commented Jun 10, 2018

If __array_function__ becomes a thing, then it would certainly make sense for scipy to use it here. But there's no reason to hold up adding separate functions/methods for it.

@rth
Copy link
Contributor

rth commented Jun 28, 2019

If array_function becomes a thing, then it would certainly make sense for scipy to use it here. But there's no reason to hold up adding separate functions/methods for it.

Even if one makes np.nanmin work on sparse arrays, I imagine having a method or a function in scipy.sparse would still be useful for discoverability?

Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

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

There is a merge conflict that would need resolving.

scipy/sparse/tests/test_base.py Outdated Show resolved Hide resolved
scipy/sparse/tests/test_base.py Outdated Show resolved Hide resolved
scipy/sparse/data.py Outdated Show resolved Hide resolved
scipy/sparse/data.py Outdated Show resolved Hide resolved
@perimosocordiae
Copy link
Member

@glemaitre this PR is looking close to mergeable, just a few minor issues to address. If you don't think you'll have time, let us know and someone else can take over (your existing commits will remain).

@perimosocordiae perimosocordiae added the needs-work Items that are pending response from the author label Jul 9, 2019
@glemaitre
Copy link
Contributor Author

@perimosocordiae Be aware that right now, nanmin nanmax are just methods. I am unsure what's best between methods and functions then.

@perimosocordiae
Copy link
Member

I just opened gh-18542 which preserves these commits and resolves the merge conflict from scipy/sparse/data.py, so I'll close this PR now.

perimosocordiae added a commit that referenced this pull request May 27, 2023
* EHN: add nanmin/nanmax in scipy.sparse

* DOC cross-referencing to other docstring

* PEP8

* address comments

* DOC update docstring

* fix doc

* Fix test failure

* Apply suggestions from code review

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

* Fix tests

* Fix test error missed in the rebase

---------

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement needs-work Items that are pending response from the author scipy.sparse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants