Adds runtime warning on divide-by-zero, fixes use of "out" and "where" in arctan2 #5204
Conversation
drculhane
force-pushed
the
Closes-5132
branch
from
ecb5af1 to
12f6386
Compare
| where : bool or pdarray, default=True | ||
| out : Optional, pdarray | ||
| A pdarray in which to store the result, or to use as a source when where is False. | ||
| where : Optional, bool or pdarray, default=True |
There was a problem hiding this comment.
Here where default=True, but above default=None. These should match.
There was a problem hiding this comment.
See also "Default set to True." later in the docstring.
There was a problem hiding this comment.
Also, where should accept bool_scalars, not just bool. That would match what NumPy allows. Also, this will change your error statements below.
|
|
||
| # Begin with various checks. | ||
|
|
||
| if out is None and where is not None: |
There was a problem hiding this comment.
Make sure this matches the default you choose above (see comment) or this could auto-fail.
| # The line below is needed in order to get the "where" function without | ||
| # a name conflict, since "where" is also a parameter name. | ||
|
|
||
| ak_where = sys.modules[__name__].where |
There was a problem hiding this comment.
Does from arkouda.numpy.numeric import where as ak_where work? That would be a lot better.
| ts = resolve_scalar_dtype(denom) | ||
| if isinstance(x1, pdarray): | ||
| ndim = x1.ndim | ||
| elif isinstance(x2, pdarray): |
There was a problem hiding this comment.
Ending an if block with elif is confusing b/c it implies there is a third option. Either change this to an else or add an else that throws an error.
|
|
||
| else: | ||
| return scalar_array(arctan2(num, denom) if where else num / denom) | ||
| else: # should be impossible to reach here. |
There was a problem hiding this comment.
I don't like the comment "should be impossible to reach here.". If it's truly impossible it should throw an error. If it's not impossible, the comment should be removed.
There was a problem hiding this comment.
Actually, what I like better is allow scalar/scalar to the inputs, as mentioned in my previous comment.
| f"Unsupported types {type(num)} and/or {type(denom)}. Supported " | ||
| f"Unsupported types {type(x1)} and/or {type(x2)}. Supported " | ||
| "types are numeric scalars and pdarrays. At least one argument must be a pdarray." | ||
| ) |
There was a problem hiding this comment.
NumPy supports scalar/scalar ops, could we just default to NumPy in this case, instead of throwing an error?
In [17]: np.arctan2(1,2)
Out[17]: np.float64(0.4636476090008061)| ret = eval(f"fcvec {op} scvec") | ||
| assert isinstance(ret, return_type) | ||
| metrics["ak_supported"] += 1 | ||
| try: |
There was a problem hiding this comment.
I don't like a try during a test, could you just set akerr.seterr(divide="ignore") instead?
2 participants
Closes #5132. Closes #5149.
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.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.Update:
Since other tests were also causing divide-by-zero errors, I extended the divide-by-zero check to use
ak.errstate. I also bypassed additional tests in datetime once a divide-by-zero occurred, because they would also cause errors.