roots-fortran icon indicating copy to clipboard operation
roots-fortran copied to clipboard

Robust sign comparison

Open ivan-pi opened this issue 3 years ago • 2 comments

https://github.com/jacobwilliams/roots-fortran/blob/60524bc4f491a50ae1b2e538e1e486551812b9db/src/root_module.F90#L1837

I noticed the sign comparison is performed inconsistently; i.e. in some places as fa*fc < 0 and in some places you extract the sign explicitly like above.

The latter option is in agreement with what Kahaner, Moler, & Nash have to say about this:

image

The original SLATEC FZERO from Shampine & Watts does it like this:

IF (SIGN(1.0E0,FB) .NE. SIGN(1.0E0,FC)) ...

Leaving the underflow issues aside, the sign version seems to generate a few instructions less than your branching isign function. Moreover, gfortran and ifx produce branchless instructions when using the sign intrinsic: https://godbolt.org/z/E41P9c4Mz

As a matter of clarity, the sign comparison could also be abstracted as a function or operator, e.g.

if (sne(fb,fc)) ...  ! sign not equal

or

if( fb .oppsign. fc) ...   ! opposite sign

but I guess this is more a matter of taste (I'm not advocating for it, but the compilers seem able to inline such operators just fine).

ivan-pi avatar Aug 04 '22 12:08 ivan-pi

Maybe i can just add this:

    pure logical function opposite_sign(x1,x2)

    real(wp),intent(in) :: x1,x2

    opposite_sign = sign(1.0_wp,x1) /= sign(1.0_wp,x2)

    end function opposite_sign

The isign function was from toms748. I need to check if there is some subtle difference because they are specifically handing the 0 case...

jacobwilliams avatar Aug 13 '22 13:08 jacobwilliams

I noticed SciPy actually had the same issue (https://github.com/scipy/scipy/issues/13737) and they introduced a sign-bit comparison to avoid the test failing due to underflow/overflow.

ivan-pi avatar Sep 15 '22 12:09 ivan-pi