Repository navigation
ENH: nan_to_num keyword addition (was #9355) - #13219
Conversation
mattip
added
01 - Enhancement
component: numpy.lib
56 - Needs Release Note.
|
LGTM, seems like a good and pretty safe change. It needs a release note. In Are others (eg, @madphysicist , @eric-wieser who commented before, plus anyone else) ok with the parameter name change to |
|
Some more minor notes: We dropped the idea of a I had a slight hesitation that the dispatcher should also return the nan, posinf and neginf parameters if not none. That could account for cases where the user supplied a scalar of a funny ducktype. But since those parameters are documented here to be scalars and not arrays, it seems better not to handle them in the dispatcher. (In my opinion, there are still unanswered questions about how ducktype'd scalars work for |
|
Did this ever hit the mailing list? |
Update release notes to include "nan_to_num" information.
|
Hi @ahaldane , Thanks for reviewing this. I added the release notes information. I hope all is fine. Also thanks to @eric-wieser @madphysicist @mattip !!! |
|
@kikocorreoso, looks good. Following @mattip, I think it's worth posting a message to the mailing list about this PR, to make sure everyone is OK with the new functionality and keyword names. (Once this PR is merged, it's very hard for us to change the keywords later because of back-compatibility, so we want to get it right at the start!) Just a short message will do, with a link to this PR and a brief description of the change and of the keyword names. The list is [email protected], you may need to subscribe to it to post. |
|
|
||
| New keywords added to ``np.nan_to_num`` | ||
| --------------------------------------- | ||
| ``np.nan_to_num`` now accepts keywords ``nan``, ``posinf`` and ``neginf`` allowing the |
There was a problem hiding this comment.
Circle-CI is complaining about "WARNING: Inline literal start-string without end-string." on this line, but I don't see the problem myself.
There was a problem hiding this comment.
it is the s on line 203, sphinx cannot parse
``code``s
| nan=10, posinf=20, neginf=30) | ||
| assert_all(vals[0] == 30) and assert_all(np.isfinite(vals[0])) | ||
| assert_(vals[1] == 10) | ||
| assert_all(vals[2] == 20) and assert_all(np.isfinite(vals[2])) |
There was a problem hiding this comment.
Can these three lines be replaced by assert_equal(vals, [30, 10, 20]) ?
There was a problem hiding this comment.
Oh I see, you were just following the pattern above. Still, I think it's better to use the shorter version if there is no problem.
There was a problem hiding this comment.
Yes, I was following the previous code.
No problem to update this.
| idx_neginf = isneginf(d) | ||
| _nx.copyto(d, nan, where=idx_nan) | ||
| _nx.copyto(d, maxf, where=idx_posinf) | ||
| _nx.copyto(d, minf, where=idx_neginf) |
There was a problem hiding this comment.
I don't see the point of splitting these 3 lines into 6?
There was a problem hiding this comment.
This is to maintain what the user has chosen. If there is only this:
_nx.copyto(d, nan, where=idx_nan)
_nx.copyto(d, maxf, where=idx_posinf)
_nx.copyto(d, minf, where=idx_neginf)
If in the keywords one chooses nan=np.inf and posinf=-999 the following array:
a = [np.nan, 10, np.inf]
would be converted to:
a = [-999, 10, -999]
So the nan keyword would be overwritten as it is the first being executed. I think this wouldn't be the expected behavior. I would expect:
a = [np.inf, 10, -999]
This way it is necessary to create temporary boolean arrays but I think it makes more sense.
There was a problem hiding this comment.
Also, this is why I've added these tests:
There was a problem hiding this comment.
Oh right, that makes sense, thanks
| assert_(result is vals) | ||
| assert_all(vals[0] == 30) and assert_all(np.isfinite(vals[0])) | ||
| assert_(vals[1] == 10) | ||
| assert_all(vals[2] == 20) and assert_all(np.isfinite(vals[2])) |
|
All right, looks good to me. Thanks @kikocorreoso , I'm going to merge now. |
3 participants
[This was #9355 ]
As I removed my local repo since #9355 was sent I send a new PR.
Sorry if this is not the canonical way. I tried to push to the old PR but I haven't found the way 😞
I hope this PR makes more sense than #9355 .
I've added tests to try to check if all is fine.
Please, let me know it all is fine or if it needs more work.