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

divide, ln, log, mod: Clarified behavior for 0 input / infinity results #473

Closed
wants to merge 4 commits into from

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Oct 24, 2023

We mention NaN here the first time, I think.

Related: #468, #472, #474

@@ -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.",
Copy link
Member

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 for x < 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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

@m-mohr m-mohr Oct 27, 2023

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 ;-)

Copy link
Member

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

log.json Show resolved Hide resolved
@m-mohr
Copy link
Member Author

m-mohr commented Oct 27, 2023

I'll actually close here and merge this into #476 because the changes are overlapping.

@m-mohr m-mohr closed this Oct 27, 2023
@m-mohr m-mohr deleted the clarify-infinity branch October 27, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants