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

Fix machine error negative values on m2 #238

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix machine error negative values on m2 #238

wants to merge 6 commits into from

Conversation

viraltux
Copy link

I found an issue with this test when m2 takes negative values due to machine error precision, see below:

x = [0.09999999999999981, 0.09999999999999978, 0.09999999999999978, 0.09999999999999978, 0.09999999999999977, 0.09999999999999983, 0.09999999999999981, 0.0999999999999998, 0.09999999999999981, 0.09999999999999978];

JarqueBeraTest(x)
ERROR: DomainError with -1.734723475976807e-18:
Exponentiation yielding a complex result requires a complex argument.
Replace x^y with (x+0im)^y, Complex(x)^y, or similar.

@@ -63,7 +63,7 @@ function JarqueBeraTest(y::AbstractVector{T}) where T<:Real
m4r += yi^4 / n
end
# compute central moments (http://mathworld.wolfram.com/CentralMoment.html)
m2 = -m1r^2 + m2r
m2 = abs(-m1r^2 + m2r)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what the (dis)advantages of this approach are compared with

Suggested change
m2 = abs(-m1r^2 + m2r)
m2 = max(-m1r^2 + m2r, 0)

Copy link
Author

@viraltux viraltux Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just about to pull a request for that option! It is better. At least mathematically more pleasant. Unless abs is faster than max... but probably not.

And now I am thinking that in the example that fails the second moment is not exactly zero which means that abs in that scenario is more precise than max that would set the second moment to exactly zero.

Now I think I should revert back to abs again! Okay, I'll sleep on it.

viraltux added 2 commits June 15, 2021 23:46
Fix machine error negative values on m2. Fix NaN when m2 equals 0. Improve performance.
@viraltux
Copy link
Author

Turns out that neither abs or max solves a NaN issue in skew and kurt when m2 equal zero.

The new change should improve performance since it calculates moments recursively replacing powers per products. In fact this approach prevents entirely negative numbers in the example that previously failed.

To deal with zeros in m2 the calculation stops and returns a default for zero JarqueBeraTest result.

viraltux added 3 commits June 16, 2021 10:06
avoid last operation machine error problems
Fix negative machine error values
@viraltux viraltux requested a review from devmotion July 5, 2021 17:54
@nalimilan
Copy link
Member

The new change should improve performance since it calculates moments recursively replacing powers per products. In fact this approach prevents entirely negative numbers in the example that previously failed.

Have you tried only replacing yi^4 with yi^2 * yi^2? Julia has special cases for powers up to 3 (literal_pow) defined in terms of x*x*..., and I would expect the compiler to be able to optimize code automatically.

@nalimilan
Copy link
Member

Also, could you add a test that fails without this PR?

@devmotion
Copy link
Member

Have you tried only replacing yi^4 with yi^2 * yi^2?

Maybe (yi^2)^2 would be more performant? Or would it be more difficult for the compiler to optimize?

@nalimilan
Copy link
Member

According to @code_native, these give the same result, so any of them is fine, but maybe yours is simpler to reason about for the compiler.

BTW, apparently precision will be better for x^4 than for other approaches, see JuliaLang/julia#20889 (comment). Not sure whether that's a problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants