egallesio / STklos

STklos Scheme
http://stklos.net
GNU General Public License v2.0
68 stars 17 forks source link

Promoting to complex before multiplying may be leading to wrong results, and `(square +inf.0+1i)` `!=` `(expt +inf.0+1i 2)` #586

Closed jpellegrini closed 3 months ago

jpellegrini commented 11 months ago

Hi @egallesio !

I've been testing STklos, since there is a 2.0 release being prepared... And I think I found a glitch.

I understand that the convert C function converts numbers up until they're compatible for operations. But there is one case in which this could be problematic. For example:

(*  2+inf.0i  2)  =>  -nan.0+inf.0i

but it should be 4+inf.0i, because the second argument has no imaginary part, and the effect of the multiplication would be to just double the real and imag parts of 2+inf.0i. However, since the number 2 is promoted to complex, it becomes 2+0i, and the multiplication of 0 by infinity results in NaN.

This is also the origin of this other problem:

(* 1 +inf.0+inf.0i) => -nan.0-nan.0i

And as a consequence:

(square +inf.0+1i) => +inf.0+inf.0i
(expt +inf.0+1i 2) => -nan.0-nan.0i

This is because (expt +inf.0+1i 2) calls exact_exponent_expt, which at the end will multiply (* 1 +inf.0+inf.0i).

I was going to propose a fix, but I'm not sure what would be the best way to deal with this without breaking the nice symmetry and generality of the convert function...

Maybe adding one more case in convert, with one more possible result, that instead of a tc_ type, would mean "I did not convert, because one of them is a complex and I'd add a zero that would be multiplied by a NaN or infinity". tc_not_boxed, maybe? Or tc_last_standard? If this is OK, I can prepare a PR.

jpellegrini commented 11 months ago

It also affects division:

(/ 2 +inf.0)      => 0.0
(/ 2 +inf.0+1i)   => -nan.0-nan.0i
(/ 2 +inf.0+0.0i) => -nan.0-nan.0i

I'm working on a patch - not sure it's the best way, but I'll try. :)

egallesio commented 5 months ago

I think PR #587 corrects all the example given. In your PR comments, you said that we should have

(/ 2 +inf.0+0.0i) => +nan-0.0i

Chez seems to have changed the result to 0.0+0.0i which seems coherent with the fact that (/ 2 +inf.0) ==> 0.0

Normally we have now always the same results that the last version of Chez (version 10.0 had a lot of modifications on this point). There is still a problem, in (/ o1 o2) if o2 contains an infinity, we always return 0.0+0.0i, but the result seems to be ±0.0±0.0i. I don't know the rule to determine the sign of each 0.

jpellegrini commented 5 months ago

Hi @egallesio ! It looks like it's all fine now! Great! :grinning:

jpellegrini commented 3 months ago

This is fixed already, I think I forgot to close the issue! Closing now...