Skip to content

Refactor: Separate Out Complex Conjugation Tests #245

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.

Sign up for GitHub

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

Closed
hemalvarambhia wants to merge 23 commits into from
Closed

Refactor: Separate Out Complex Conjugation Tests #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8967e82
Classified some complex conjugation tests with a clearly named category.
hemalvarambhia Apr 19, 2022
c6fec7d
Removed what feels like an obsolete test.
hemalvarambhia Apr 19, 2022
9b521e0
Clarified the name of a test.
hemalvarambhia Apr 19, 2022
e6c24f1
Clarified the name of a test message.
hemalvarambhia Apr 19, 2022
84d694d
Removed an obsolete test.
hemalvarambhia Apr 19, 2022
23837de
Removed some obsolete tests.
hemalvarambhia Apr 19, 2022
3a910d4
Added some missing tests that assert the properties of complex conjug…
hemalvarambhia Apr 19, 2022
999182f
Made the message more concise.
hemalvarambhia Apr 19, 2022
6f2d341
Extract computation of the complex conjugate to an intention-revealin…
hemalvarambhia Apr 19, 2022
c4d89b2
Simplified the predicate.
hemalvarambhia Apr 19, 2022
93dd8a0
Clarified the name of the test.
hemalvarambhia Apr 19, 2022
0e8b48f
Made the return statement match the selector.
hemalvarambhia Apr 19, 2022
3fcf89b
Clarified a test name.
hemalvarambhia Apr 19, 2022
cadac64
Improved code formatting.
hemalvarambhia Apr 19, 2022
99b6c0a
Made the category consistent with the tests for ComplexNumber
hemalvarambhia Apr 19, 2022
1f6365b
Exercise the complexConjugate message directly.
hemalvarambhia Apr 19, 2022
50ee46d
Removed duplicate test - this is already in PMNumberTest.
hemalvarambhia Apr 19, 2022
f5e66f1
Exercise the complexConjugate message directly.
hemalvarambhia Apr 19, 2022
d677792
Exercise complexConjugate directly.
hemalvarambhia Apr 19, 2022
4d022ca
Added tests that exercise isComplexConjugateOf directly.
hemalvarambhia Apr 19, 2022
fba37a1
Made the test name consistent.
hemalvarambhia Apr 19, 2022
1948769
Made the test names a little more concise.
hemalvarambhia Apr 19, 2022
6307f89
Made the test name more similar/consistent.
hemalvarambhia Apr 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Math-Complex/PMComplex.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,10 @@ PMComplex >> isComplexConjugateOf: aNumber [

{ #category : #testing }
PMComplex >> isComplexConjugateOfAComplexNumber: aComplexNumber [

"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
Copy link
Contributor Author

@hemalvarambhia hemalvarambhia Apr 19, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

@olekscode olekscode Apr 22, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

Issue #247

]

{ #category : #testing }
Expand Down
136 changes: 82 additions & 54 deletions src/Math-Tests-Complex/PMComplexTest.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,60 +389,6 @@ PMComplexTest >> testHash [
self assert: aComplex copy hash equals: aComplex hash
]

{ #category : #tests }
Copy link
Member

@olekscode olekscode Apr 22, 2022

Choose a reason for hiding this comment

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

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

PMComplexTest >> testIsComplexConjugateOfConjugateComplex [

self assert: ((3 + 2i) isComplexConjugateOf: (3 - 2i))
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfConjugateComplexAndReal [

self assert: ((5 + 0i) isComplexConjugateOf: 5)
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfConjugateRealAndComplex [

self assert: (5 isComplexConjugateOf: (5 - 0i))
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfDifferentReal [

self deny: (-5 isComplexConjugateOf: 5)
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfNonConjugateComplexAndReal [

self deny: ((5 + 3i) isComplexConjugateOf: 5)
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfNonConjugateDifferentComplex [

self deny: ((-0.5 - 1i) isComplexConjugateOf: (3 - 2i))
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfNonConjugateRealAndComplex [

self deny: (5 isComplexConjugateOf: (5 - 3i))
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfSameComplex [

self deny: ((3 - 2i) isComplexConjugateOf: (3 - 2i))
]

{ #category : #tests }
PMComplexTest >> testIsComplexConjugateOfSameReal [

self assert: (5 isComplexConjugateOf: 5)
]

{ #category : #tests }
PMComplexTest >> testIsComplexNumberOnComplex [

Expand Down Expand Up @@ -783,6 +729,72 @@ PMComplexTest >> testTanh [
self assert: (c2 imaginary closeTo: c tanh imaginary).
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugateHasEqualRealPartAndAnImaginaryPartWithEqualMagnitudeButOppositeSign [
Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hemalvarambhia hemalvarambhia Apr 23, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@hemalvarambhia hemalvarambhia Apr 23, 2022

Choose a reason for hiding this comment

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

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

| z |
z := 3 + 2 i.

self assert: z complexConjugate equals: 3 - 2 i
Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

To sum up, I propose the following:

testComplexConjugate

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

Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

I will open a separate issue for that

Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

Issue #246

]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugateOfAPureRealNumberIsItself [
| z |
z := 5 + 0 i.

self assert: z complexConjugate equals: 5.
Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

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:

  1. 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
  2. Add complexConjugate to Number class (see my first comment to this review)
  3. Write the following test for Number:
NumberTest >> testComplexConjugate

	self assert: 5 complexConjugate equals: 5

]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugationIsAnInvolution [
| z |
z := -9 + 24 i.

self assert: z complexConjugate complexConjugate equals: -9 + 24 i
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugationIsDistributiveOverAddition [
| z w |
z := -3 + 6 i.
w := 10 - 4 i.

self assert: (z + w) complexConjugate equals: (z complexConjugate + w complexConjugate)
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugationIsDistributiveOverDivision [
| z w |
z := 6 - 15 i.
w := 10 + 5 i.

self assert: (z / w) complexConjugate equals: (z complexConjugate / w complexConjugate)
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugationIsNotSymmetric [
| z |
z := 3 - 2 i.

self deny: z complexConjugate equals: z
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatComplexConjugationPreservesModulus [
| z |
z := 3 - 4 i.

self assert: z complexConjugate abs equals: 5
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testThatTwoEntirelyDifferentComplexNumbersAreNotComplexConjugatesOfEachOther [
| z |
z := -0.5 - 1 i.

self deny: z complexConjugate equals: (3 - 2i)
Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

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

]

{ #category : #tests }
PMComplexTest >> testTimesPolynomial [
| c poly |
Expand Down Expand Up @@ -810,6 +822,22 @@ PMComplexTest >> testTwoComplexNumbersWithDifferentRealPartsAreNotEqual [
self deny: z equals: w.
]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testWeCanIdentifyTwoComplexNumbersAsBeingComplexConjugatesOfEachOther [
| z |
z := 3 - 2 i.

self assert: (z isComplexConjugateOf: (3 + 2 i))
Copy link
Member

@olekscode olekscode Apr 23, 2022

Choose a reason for hiding this comment

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

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

]

{ #category : #'testing - complex conjugation' }
PMComplexTest >> testWeCanIdentifyTwoComplexNumbersAsNotBeingComplexConjugatesOfEachOther [
| z |
z := -4 - 8 i.

self deny: (z isComplexConjugateOf: 13 + 12 i)
]

{ #category : #'testing - expressing complex numbers' }
PMComplexTest >> testWeCanWriteComplexNumbersWhoseRealAndImaginaryPartsAreFractions [
| z |
Expand Down
6 changes: 3 additions & 3 deletions src/Math-Tests-Complex/PMNumberTest.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Class {
#category : #'Math-Tests-Complex'
}

{ #category : #'complex conjugation' }
{ #category : #'testing - complex conjugation' }
PMNumberTest >> testComplexConjugateOfAnIntegerIsAnInteger [
|complexConjugateOfInteger|

Expand All @@ -13,15 +13,15 @@ PMNumberTest >> testComplexConjugateOfAnIntegerIsAnInteger [
self assert: complexConjugateOfInteger equals: -5.
]

{ #category : #'complex conjugation' }
{ #category : #'testing - complex conjugation' }
PMNumberTest >> testComplexConjugateOfRealFractionIsARealFraction [
| complexConjugateOfFraction |
complexConjugateOfFraction := (Fraction numerator: 1 denominator: 6) complexConjugate.

self assert: complexConjugateOfFraction equals: (Fraction numerator: 1 denominator: 6) .
]

{ #category : #'complex conjugation' }
{ #category : #'testing - complex conjugation' }
PMNumberTest >> testComplexConjugateOfRealNumberIsItself [
|realNumber|
realNumber := 4.5 complexConjugate.
Expand Down