-
Notifications
You must be signed in to change notification settings - Fork 16
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
divide, ln, log, mod: Clarified behavior for 0 input / infinity results #473
Conversation
@@ -1,7 +1,7 @@ | |||
{ | |||
"id": "divide", | |||
"summary": "Division of two numbers", | |||
"description": "Divides argument `x` by the argument `y` (*`x / y`*) and returns the computed result.\n\nNo-data values are taken into account so that `null` is returned if any element is such a value.\n\nThe computations follow [IEEE Standard 754](https://ieeexplore.ieee.org/document/8766229) whenever the processing environment supports it. Therefore, a division by zero results in ±infinity if the processing environment supports it. Otherwise, a `DivisionByZero` exception must the thrown.", | |||
"description": "Divides argument `x` by the argument `y` (*`x / y`*) and returns the computed result.\n\nNo-data values are taken into account so that `null` is returned if any element is such a value.\n\nThe computations follow [IEEE Standard 754](https://ieeexplore.ieee.org/document/8766229) whenever the processing environment supports it. A division by zero results in:\n\n- +infinity for `x` > 0,\n- -infinity for `x` < 0,\n- `NaN` for `x` = 0,\n- or otherwise, throws a `DivisionByZero` exception if the other options are not supported by the processing environment.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+infinity for
x
> 0,\n- -infinity forx
< 0,\n-
I'm not sure this is correct (I'm also not sure how tightened down the spec should be here):
the sign of infinity also depends on which side you're looking from, e.g. 1/y
at y=0
is +infinity if you "approach" that limit it from positive y values, but -infinity if you approach it from negative values.
I think there is a bit of discrepancy between rigid mathematical definitions and what is practically feasible in a numerical/computer implementation, so I would not over-specify this. I'm kind of fine with what we had originally, or something like
a division by zero results in ±infinity or NaN, depending on the capabilities of the processing environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if we should recommend against (or forbid) throwing a DivisionByZero error. If you're processing thousands/millions of pixels you probably prefer a NaN in some output pixels over your whole job failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't specify ±Infinity in tests though, that's why I created this PR overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if we should recommend against (or forbid) throwing a DivisionByZero error. If you're processing thousands/millions of pixels you probably prefer a NaN in some output pixels over your whole job failing.
Well, the error only occurs if your environment doesn't support NaN/Infinity. It's difficult to return NaN if NaN is unsupported ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's difficult to return NaN if NaN is unsupported
sure, but we could recommend against throwing DivisionByZero if the environment does support NaN
I'll actually close here and merge this into #476 because the changes are overlapping. |
We mention NaN here the first time, I think.
Related: #468, #472, #474