-
Notifications
You must be signed in to change notification settings - Fork 22
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
fix xexpy(0, 1000)
#80
Conversation
Fixes #79 |
Maybe add xexpy(0, 1000) as a test case? |
tests added. |
Could we get a release with this? |
The PR does not match what |
@devmotion I don't understand what you mean? |
To elaborate a bit more, |
This PR maintains
|
Why? This requirement is not documented. I would also note that |
The behaviour of each function is documented which implicitly documents the invariance.
My comment was supposed to be understood as a exact/mathematical description of the intended functions and their relation, disregarding floating-point issues such as overflow. That is, the invariance is for the mathematical functions underlying |
In that case, isn't this PR correct? This PR leaves xexpy(0, Inf) as NaN. |
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 missed the check for isfinite(y)
, so I think generally the PR is fine. It still disagrees with the docstring though since due to the change xexpy(x, y)
is not always x * exp(y)
if y != -Inf
anymore. Can you adjust the docstring (e.g., by changing if `y == -Inf`
to if `y == -Inf` or if `x == 0.0` and `y` is finite.
) and maybe also add the xexpy(0, 1000)
example as a doctest?
I interpret the docstring as describing the function mathematically, in which case it is already correct. |
Generally, the docstrings in LogExpFunctions mention explicitly if the computation deviates from the given Julia code ("preserving numerical accuracy", "carefully evaluated", "avoiding over/underflow" ...), so IMO the docstring should either mention the additional special case or contain a comment that indicates that the computation is not just |
Ideally this code could avoid more overflow to deal with cases like |
Co-authored-by: David Widmann <[email protected]>
Could this PR include a version bump? I would like to use this updated |
now it equals 0.