Refactor: Separate Out Complex Conjugation Tests #245

hemalvarambhia · 2022-04-19T16:48:47Z

Clarified test naming and grouped the tests into a intention-revealing category.

Added new tests that demonstrates the properties of complex conjugation e.g. involution, distributive.

…ation.

…g temporary variable.

src/Math-Tests-Complex/PMComplexTest.class.st

src/Math-Complex/PMComplex.class.st

hemalvarambhia · 2022-04-19T19:43:47Z

src/Math-Complex/PMComplex.class.st

 	"Answer true if self and aComplexNumber are complex conjugates of each other. The complex conjugate of a complex number is the number with an equal real part and an imaginary part equal in magnitude but opposite in sign."
-	^ (aComplexNumber real = real) and: [ aComplexNumber imaginary = (-1 * imaginary) ]
+
+	^ self = aComplexNumber complexConjugate


Does this need to a method? It's only used by a Matrix to check it is Hermitian. I don't feel it's needed. Is there evidence to the contrary?

Yes, this method is needed for double dispatch.
I added it in this commit: 7d77acf

The idea is that both Number and PMComplex should answer to the message isComplexConjugateOf:
Because we need to apply it to matrices where we do not know if the values are real or complex

That being said, perhaps, we could simply define a complexConjugate method for the Number class
It would always return self

Then instead of isComplexConjugateOf: inside PMMatrix >> isHermitian, we could write number = otherNumber complexConjugate

I will open a separate issue for that

olekscode

I propose to reject this PR. Please see my comments

olekscode · 2022-04-22T23:51:14Z

src/Math-Complex/PMComplex.class.st

 	"Answer true if self and aComplexNumber are complex conjugates of each other. The complex conjugate of a complex number is the number with an equal real part and an imaginary part equal in magnitude but opposite in sign."
-	^ (aComplexNumber real = real) and: [ aComplexNumber imaginary = (-1 * imaginary) ]
+
+	^ self = aComplexNumber complexConjugate


Yes, this method is needed for double dispatch.
I added it in this commit: 7d77acf

The idea is that both Number and PMComplex should answer to the message isComplexConjugateOf:
Because we need to apply it to matrices where we do not know if the values are real or complex

That being said, perhaps, we could simply define a complexConjugate method for the Number class
It would always return self

Then instead of isComplexConjugateOf: inside PMMatrix >> isHermitian, we could write number = otherNumber complexConjugate

I will open a separate issue for that

olekscode · 2022-04-22T23:57:22Z

src/Math-Tests-Complex/PMComplexTest.class.st

 	self assert: aComplex copy hash equals: aComplex hash
 ]

-{ #category : #tests }


This doesn't seem like refactoring.
Those tests are completely different from the ones that you added

olekscode · 2022-04-23T00:03:51Z

src/Math-Tests-Complex/PMComplexTest.class.st

 ]

+{ #category : #'testing - complex conjugation' }
+PMComplexTest >> testThatComplexConjugateHasEqualRealPartAndAnImaginaryPartWithEqualMagnitudeButOppositeSign [


Those long test names are very hard to read even if I can see all of it (and System Browser will hide most of the name because of the window size).

testThatComplexConjugateHasEqualRealPartAndAnImaginaryPartWithEqualMagnitudeButOppositeSign

I really prefer this name:

testComplexConjugate

Why?
Because we are testing if complexConjugate method returns the correct complex conjugate.
That's it.
Saying that "it must have equal real part and an imaginary part with equal magnitude but opposite sign" is just the definition of a complex conjugate. This sentence should be in a documentation comment of the complexConjugate method but not in the test name

There is a bit more to complex conjugation e.g. it is distributive over the binary operations for example hence:

I would say testWeCanComputeTheComplexConjugate

but if you insist I'm happy to go with what you suggest.

Also, there is the value or idea of Tests as documentation c.f. XUnit Test Patterns by Géerard Mezsaros

olekscode · 2022-04-23T00:08:40Z

src/Math-Tests-Complex/PMComplexTest.class.st

+	| z |
+	z := 3 + 2 i.
+
+	self assert: z complexConjugate equals: 3 - 2 i


I think that the code would be more readable if we don't create a variable here:

self assert: (3 + 2i) complexConjugate equals: (3 - 2i).

Complex numbers are native to Pharo, they should be used like any other numbers.
And personally, I wouldn't write this:

| a b | a := 4. b := -4. self assert: a negated equals: b.

I would just write

self assert: 4 negated equals: -4

So the same logic should apply to complex numbers

To sum up, I propose the following:

testComplexConjugate self assert: (3 + 2i) complexConjugate equals: (3 - 2i).

The method with that name already exists but it is not good...

testComplexConjugate | z complexConjugateOfZ expected | z := 5 - 6 i. complexConjugateOfZ := z complexConjugate . expected := 5 + 6 i. self assert: complexConjugateOfZ equals: expected.

I propose to replace it with a more readable one-line version:

testComplexConjugate self assert: (5 - 6i) complexConjugate equals: (5 + 6i).

I will open a separate issue for that

olekscode · 2022-04-23T00:25:15Z

src/Math-Tests-Complex/PMComplexTest.class.st

+	| z |
+	z := 5 + 0 i.
+
+	self assert: z complexConjugate equals: 5.


This test is not good.

First, the same comment about test name as above.
I would use a shorter name: testComplexConjugateOfRealNumber
The fact that it's itself is a definition of complex conjugate and should be in the method comment and not the test name

Second, about the temporary variable, same comment as above - I would not use it because it is unnecessary and make the code harder to read (of course, this is subjective).

Third (!), this test is misleading.
(5 + 0i) complexConjugate does not return 5 (a real number)
It returns (5 + 0i) (a complex number)

The reason why this test passes is because (5 + 0i) = 5

I would do the following:

Remove this test, because for complex numbers, testing that self assert: (5 - 0i) complexConjugate equals (5 + 0i) is the same test as self assert: (3 + 2i) complexConjugate equals (3 - 2i). And we already have that test as testComplexConjugate

Add complexConjugate to Number class (see my first comment to this review)

Write the following test for Number:

NumberTest >> testComplexConjugate self assert: 5 complexConjugate equals: 5

olekscode · 2022-04-23T00:30:36Z

src/Math-Tests-Complex/PMComplexTest.class.st

+	| z |
+	z := -0.5 - 1 i.
+
+	self deny: z complexConjugate equals: (3 - 2i)


I would remove ALL those tests.
They pass by definition if testComplexConjugate passes

There is no need to pollute the code with 6 tests that can not possibly fail
The only way this can fail is either:

(a) if testComplexConjugate fails
(b) if complex numbers don't behave as expected, but this should be (an it is) reflected in the tests of complex numbers. It has nothing to do with complex conjugates specifically

olekscode · 2022-04-23T00:40:25Z

src/Math-Tests-Complex/PMComplexTest.class.st

+	| z |
+	z := 3 - 2 i.
+
+	self assert: (z isComplexConjugateOf: (3 + 2 i))


For the same reasons as discussed above, I would name this test testIsComplexConjugateOf and I would not create a temporary variable. So the code would be:

testIsComplexConjugateOf self assert: ((3 - 2i) isComplexConjugateOf: (3 + 2i)).

But this was already in the tests that you propose to remove.

Also, look at those tests that you propose to remove... they cover many important scenario:

is complex conjugate of complex and real

is complex conjugate of real and complex (different receiver)

is complex conjugate of complex and complex

is complex conjugate of complex and complex when they are not complex conjugate of each other
...

If we keep the isComplexConjugateOf: method, we must keep all those tests.
But if we remove it, and add complexConjugate to Number as I proposed above (will be a separate issue), then we can remove them and simply add testComplexConjugate to both Number and PMComplex

hemalvarambhia added 7 commits April 19, 2022 17:17

Classified some complex conjugation tests with a clearly named category.

8967e82

Removed what feels like an obsolete test.

c6fec7d

Clarified the name of a test.

9b521e0

Clarified the name of a test message.

e6c24f1

Removed an obsolete test.

84d694d

Removed some obsolete tests.

23837de

Added some missing tests that assert the properties of complex conjug…

3a910d4

…ation.

hemalvarambhia marked this pull request as ready for review April 19, 2022 16:50

hemalvarambhia added Status: Review Needed Refactoring labels Apr 19, 2022

hemalvarambhia changed the title ~~Refactor complex number tests~~ Refactor: Clarify Complex Number Complex Conjugation Tests Apr 19, 2022

hemalvarambhia added 3 commits April 19, 2022 18:17

Made the message more concise.

999182f

Extract computation of the complex conjugate to an intention-revealin…

6f2d341

…g temporary variable.

Simplified the predicate.

c4d89b2

hemalvarambhia requested review from Ducasse, SergeStinckwich, khinsen and olekscode April 19, 2022 17:44

hemalvarambhia commented Apr 19, 2022

View reviewed changes

src/Math-Tests-Complex/PMComplexTest.class.st Outdated Show resolved Hide resolved

Clarified the name of the test.

93dd8a0

hemalvarambhia commented Apr 19, 2022

View reviewed changes

src/Math-Complex/PMComplex.class.st Outdated Show resolved Hide resolved

hemalvarambhia added 9 commits April 19, 2022 19:11

Made the return statement match the selector.

0e8b48f

Clarified a test name.

3fcf89b

Improved code formatting.

cadac64

Made the category consistent with the tests for ComplexNumber

99b6c0a

Exercise the complexConjugate message directly.

1f6365b

Removed duplicate test - this is already in PMNumberTest.

50ee46d

Exercise the complexConjugate message directly.

f5e66f1

Exercise complexConjugate directly.

d677792

Added tests that exercise isComplexConjugateOf directly.

4d022ca

hemalvarambhia added 3 commits April 19, 2022 20:19

Made the test name consistent.

fba37a1

Made the test names a little more concise.

1948769

Made the test name more similar/consistent.

6307f89

hemalvarambhia commented Apr 19, 2022

View reviewed changes

hemalvarambhia changed the title ~~Refactor: Clarify Complex Number Complex Conjugation Tests~~ Refactor: Separate Out Conjugation Tests Apr 20, 2022

hemalvarambhia changed the title ~~Refactor: Separate Out Conjugation Tests~~ Refactor: Separate Out Complex Conjugation Tests Apr 20, 2022

olekscode requested changes Apr 23, 2022

View reviewed changes

hemalvarambhia closed this Apr 23, 2022

hemalvarambhia deleted the refactor-complex-number-tests branch April 23, 2022 06:04

Uh oh!

Refactor: Separate Out Complex Conjugation Tests #245

Refactor: Separate Out Complex Conjugation Tests #245

Uh oh!

Conversation

hemalvarambhia commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olekscode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hemalvarambhia commented Apr 19, 2022 •

edited

Loading