Adds runtime warning on divide-by-zero, fixes use of "out" and "where" in arctan2 #5315
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5315 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 4
Lines ? 63
Branches ? 0
========================================
Hits ? 63
Misses ? 0
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
drculhane
force-pushed
the
Alternate-Closes-5132
branch
from
8864961 to
566cc9d
Compare
arkouda/numpy/numeric.py
Outdated
| if _where is None or _where is True: | ||
| if out is not None: | ||
| out[:] = tmp | ||
| return tmp |
There was a problem hiding this comment.
I think this should return out instead of tmp.
There was a problem hiding this comment.
Here's an example showing NumPy behavior:
In [10]: import numpy as np
...:
...: x = np.array([1.0, -1.0])
...: y = np.array([1.0, 1.0])
...:
...: out = np.empty_like(x)
...:
...: ret = np.arctan2(x, y, out=out)
...:
...: print("ret is out:", ret is out)
...: print("ret:", ret)
...: print("out:", out)
...:
ret is out: True
ret: [ 0.78539816 -0.78539816]
out: [ 0.78539816 -0.78539816]There was a problem hiding this comment.
But here's what I get from arkouda:
In [12]: import numpy as np
...:
...: x = ak.array([1.0, -1.0])
...: y = ak.array([1.0, 1.0])
...:
...: out = ak.array(np.empty_like(x.to_ndarray()))
...:
...: ret = ak.arctan2(x, y, out=out)
...:
...: print("ret is out:", ret is out)
...: print("ret:", ret)
...: print("out:", out)
ret is out: False
ret: [0.785398 -0.785398]
out: [0.785398 -0.785398]
arkouda/numpy/numeric.py
Outdated
| return is_supported_number(arg) or is_supported_bool(arg) | ||
|
|
||
| def _bool_case(x1, x2): | ||
| return type(x1) in (bool, np.bool_) and type(x2) in (bool, np.bool) |
There was a problem hiding this comment.
np.bool is deprecated and we should use np.bool_ instead.
There was a problem hiding this comment.
It should use isinstance also. Something like this:
def _bool_case(x1, x2):
return isinstance(x1, (bool, np.bool_)) and isinstance(x2, (bool, np.bool_))There was a problem hiding this comment.
That was a typo, sure enough.
|
|
||
|
|
||
| def handle_bools(x): | ||
| if x.dtype in (bool, np.bool_, ak_bool): |
There was a problem hiding this comment.
x.dtype can never be the python bool, and ak_bool == np.bool_, so this should be x.dtype == ak_bool (or np.bool_). x.dtype == "bool" would also work.
| @pytest.mark.parametrize("num_scalar", [True, False]) | ||
| @pytest.mark.parametrize("denom_scalar", [True, False]) | ||
| @pytest.mark.parametrize("uses_where, uses_out", WHERE_OUT) | ||
| def test_arctan2(self, num_type, denom_type, num_scalar, denom_scalar, uses_out, uses_where): |
There was a problem hiding this comment.
The order of the arguments need to match the parameterization, otherwise pytest may not be doing what you think it's doing. In any case, I find it confusing. So uses_where should come before uses_out in the parameters.
arkouda/numpy/numeric.py
Outdated
| if _where is None or _where is True: | ||
| if out is not None: | ||
| out[:] = tmp | ||
| return tmp |
There was a problem hiding this comment.
Here's an example showing NumPy behavior:
In [10]: import numpy as np
...:
...: x = np.array([1.0, -1.0])
...: y = np.array([1.0, 1.0])
...:
...: out = np.empty_like(x)
...:
...: ret = np.arctan2(x, y, out=out)
...:
...: print("ret is out:", ret is out)
...: print("ret:", ret)
...: print("out:", out)
...:
ret is out: True
ret: [ 0.78539816 -0.78539816]
out: [ 0.78539816 -0.78539816]
arkouda/numpy/numeric.py
Outdated
| if _where is None or _where is True: | ||
| if out is not None: | ||
| out[:] = tmp | ||
| return tmp |
There was a problem hiding this comment.
But here's what I get from arkouda:
In [12]: import numpy as np
...:
...: x = ak.array([1.0, -1.0])
...: y = ak.array([1.0, 1.0])
...:
...: out = ak.array(np.empty_like(x.to_ndarray()))
...:
...: ret = ak.arctan2(x, y, out=out)
...:
...: print("ret is out:", ret is out)
...: print("ret:", ret)
...: print("out:", out)
ret is out: False
ret: [0.785398 -0.785398]
out: [0.785398 -0.785398]
arkouda/numpy/numeric.py
Outdated
| return is_supported_number(arg) or is_supported_bool(arg) | ||
|
|
||
| def _bool_case(x1, x2): | ||
| return type(x1) in (bool, np.bool_) and type(x2) in (bool, np.bool) |
There was a problem hiding this comment.
It should use isinstance also. Something like this:
def _bool_case(x1, x2):
return isinstance(x1, (bool, np.bool_)) and isinstance(x2, (bool, np.bool_))|
|
||
| # np.arctan2 returns float16 if x1 and x2 are both bool. This helper converts it to float64 | ||
| # because subsequent functions such as where can't accomodate float16. | ||
|
|
There was a problem hiding this comment.
It's spelled "accommodate".
2 participants
Closes #5132 and #5149.
This takes the place of PR #5204.
The binops in pdarrayclass.py now give a RuntimeWarning or a Floating Point Error (or nothing) if / or // cause a divide-by-zero, depending on the "divide" setting of ak.errstate.
Because the RuntimeWarning surfaced an issue in
arctan2, I've rewritten that function somewhat. It now has an "out" parameter matching numpy, and uses the "where" parameter correctly.arctan2 now also accepts bool inputs, which requires special handling. np.arctan2, if given bool inputs, returns np.float16, which arkouda doesn't like.
Most of the original arctan2 function remains as
_arctan2_, because (subjective opinion here) I think that calling it as a separate function improves the readability of the code.In doing this rewriting, I removed some mypy ignores that are no longer needed.
I rewrote the unit tests for
arctan2to use our newer assert functions. I think this cleans up that code significantly. In the cases that use "where," I check both the returned result, and the returned "out" pdarray, since they should match.