add raisedTo method for PMMatrix #209
Conversation
A raisedTo: anInteger method is added for PMMatrix. Now, exponentiation of matrices can be performed using this method.
src/Math-Matrix/PMMatrix.class.st
Outdated
| self assert: self numberOfRows = self numberOfColumns description: 'Matrix should be square'. | ||
|
|
||
| anInteger <= 0 ifTrue: [ | ||
| anInteger = 0 ifTrue: [^ PMMatrix identity: self numberOfRows ] |
There was a problem hiding this comment.
Nested ifs can impact comprehensibility. Perhaps, makes this a guard clause all on its own for a power = 0. Assuming you've not considered this already, is there a way we may avoid chains ifTrue/ifFalse, as I personally am finding it hard to follow
There was a problem hiding this comment.
@hemalvarambhia I have reformatted it now. Can you please check it?
src/Math-Matrix/PMMatrix.class.st
Outdated
| ] | ||
|
|
||
| { #category : #operation } | ||
| PMMatrix >> raisedTo: anInteger [ |
There was a problem hiding this comment.
| PMMatrix >> raisedTo: anInteger [ | |
| PMMatrix >> raisedTo: power [ |
There was a problem hiding this comment.
I will approve once my suggestions have been addressed. I would prefer the selector argument was renamed to power (index is another commonly used word 'power of indicies') and for there to be fewer nested ifs, as it would help me with my need to understand the code easily. The rest are formatting suggestions
| m printOn: stream | ||
| ] | ||
|
|
||
| { #category : #tests } |
There was a problem hiding this comment.
These are great tests! Well done. My only suggestions are formatting to Arrange Act Assert.
| aPMMatrix := PMMatrix rows: #(#(3 1) #(1 1)). | ||
|
|
||
| aPMMatrix := aPMMatrix raisedTo: -2. | ||
| expected := PMMatrix rows: #(#(0.5 -1) #(-1 2.5)). |
There was a problem hiding this comment.
Move the expected to be close to the self assert...
| aPMMatrix := PMMatrix rows: #(#(3 1) #(1 1)). | ||
|
|
||
| aPMMatrix := aPMMatrix raisedTo: 3. | ||
| expected := PMMatrix rows: #(#(34 14) #(14 6)). |
There was a problem hiding this comment.
Same suggestion here - move the expected closer to self assert ...
| aPMMatrix := PMMatrix rows: #(#(3 1) #(1 1)). | ||
|
|
||
| aPMMatrix := aPMMatrix raisedTo: 0. | ||
| expected := aPMMatrix. |
There was a problem hiding this comment.
Same suggestion here.
Some formatting changes, renaming arguments are also done
|
@hemalvarambhia I have made the modifications. Can you please review it when you are free? |
hemalvarambhia
left a comment
There was a problem hiding this comment.
See if you can appeal to iteration over recursion to speed things up
src/Math-Matrix/PMMatrix.class.st
Outdated
| ^ PMMatrix identity: self numberOfRows ]. | ||
|
|
||
| aPower > 0 ifTrue: [ | ||
| ^ self * (self raisedTo: aPower -1) ]. |
There was a problem hiding this comment.
Recursion! Is there a way to iterate instead in order to make it faster? I misread this due to a lack of space between the '-' and '1'
There was a problem hiding this comment.
@hemalvarambhia Yes, I could change the recursion to iteration. I had kept recursion because I felt that the code looked more concise and elegant.
hemalvarambhia
left a comment
There was a problem hiding this comment.
Approved. Worth investigating iteration over recursion for speed.
@hemalvarambhia I have replaced recursion with iteration now. |
|
@rakki-18 great job. It's great you did it so quickly. It's approved so we'll merge soon... |
Thanks a lot! |
A
raisedTo: anIntegermethod is added for PMMatrix. Now, exponentiation of matrices can be performed using this method.Eg: You can calculate A3 using
A raisedTo: 3.