PMComplex miss a signBit method (used in Pharo) Fix

[SergeStinckwich]

No description provided.

[SergeStinckwich]

Pharo 6.1 builds are red on another random test: #70
Pharo 7.0 builds are green now for #68

[hemalvarambhia]

For my own education, what drives the need for this method?

[hemalvarambhia]

I think, as this is a one line block, we might inline this,

PMComplex >> signBit [ ^ self real signBit ]

unless what you've done is the idiomatic way.

[SergeStinckwich]

I don't understand ? This is not a block. You are looking at Tonel files, the real method is :
MComplex >> signBit
^ self real signBit

[nicolas-cellier-aka-nice]

I wonder why we need to define the sign of a complex at all???
(be it signBit ot just sign does not matter)

[SergeStinckwich]

I don't know exactly ... In Pharo 7.0, arcTan: has been redesigned to use signBit instead of sign (in order to detect IEEE-754 negative-zero). PMComplex miss this method and test like PMComplex>>testArcTanDenominator fails. Maybe you have a better explanation ? :-)

[nicolas-cellier-aka-nice]

Standards (ANSI Smalltalk?) mandates that 0 sign -> 0, same for +0.0 and -0.0...
So, if we want to be compatible with other Smalltalks (Gemstone in this case) we must not use sign for detecting negativeZero... We collectively opted for signBit after a discussion (I did that in Squeak too).

But that applies to numbers, not complex numbers...
A complex number has a sign for its real part and its imaginary part too, so choosing one is quite arbitrary...

Introducing functions/operations that are not natural, but are rather hacks for making some other part of a software work is dangerous.

One good example is how Matlab defined relational operators for complex (< <= >= >).
There is no natural order, one such that (a < b and: [c < d]) ==> (a+c < (b+d)).
My experience is that programs instead of failing when we have a complex instead of a real just continue and answer a wrong result...

For what added value? I know of a single usage for sorting complex values: separate stable poles from unstable poles in some algorithms (for example solving a Riccati). For a discrete or continuous system, that's not necessarily the same sort function... (either real part > 0, or module > 1).
It's far better to define the specific sort order where we need it in this case.

That's why I ask for complex sign...

[SergeStinckwich]

I don't want to add a sign on Complex ... there was already a sign method and there was a failing test :

testArcTanDenominator
	| c1 c2 |
	c1 := 1 i.
	c2 := 0.
	self assert: (c1 arcTan: c1) equals: Float pi / 4.
	self assert: (c2 arcTan: c1) equals: 0.
	self assert: (c2 arcTan: c1 * c1) equals: Float pi

I dunno who wrote this test ?

[SergeStinckwich]

If I remove signBitmethod, I have this DNU:

[nicolas-cellier-aka-nice]

Yes, I understand that.
Locally, we had/used a sign, we must now have/use a signBit instead for latest Squeak/Pharo compatibility.
So on one hand, problem solved, issue should be closed.

But on the other hand, while at fixing it, I'm saying that rather than hacking a sign/signBit that will not be usefull outside of arcTan:, and even worse might be surprising in other contexts, and/or hide bugs or delay bug discovery, we might better fix the complex extension of arcTan:
How is 4-quadrant arc-tangent at all defined in Complex?

a + b i arctan: c + d i

Does complex extension fits real function when imaginary parts are null?

(a + 0 i arcTan: c + 0 i)  = (a arcTan: c).

What happens when one or both imaginary part is a negativeZero?

I might be able to answer to above questions since I probably implemented those...

But does the arcTan: implemented in Number works the same as Complex extension when a Complex parameter is provided? Does it use double dispatching for that?

(a arcTan: c + d i)  = (a + 0 i arcTan: c + d i).

Are there tests for all these cases?

[SergeStinckwich]

I thought PMCompelx>>arcTan: was your code ? 😄

[SergeStinckwich]

Can you move discussion on a new issue ?

[nicolas-cellier-aka-nice]

Yes, I once thought this could be discussed in a new issue.

But if Number>>arcTan: has not been patched for being compatible with complex extension of arcTan:, as I think then the correct fix is:

• either to remove the test (or change it as shouldRaise: Error) and remove Complex>>sign
• or to fix Number>>arcTan: to make it compatible with Complex>>arcTan:, for example by using double dispatching (but the change must be made in Squeak/Pharo because that's not a PM method)