-
Notifications
You must be signed in to change notification settings - Fork 76
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.
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
feat: included Matrix value setters #266
Conversation
@@ -78,6 +78,18 @@ class Matrix2 { | |||
factory Matrix2.rotation(double radians) => | |||
Matrix2.zero()..setRotation(radians); | |||
|
|||
/// Sets the value for row 0 and column 0. | |||
set r0c0(double value) => _m2storage[0] = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to provide getters to?
One could argue that get r0c0
is more performant than setEntry(0, 0, 1)
. (cc: @rakudrama )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this provide any benefits over setEntry(x, y, value)
?
Won't this just be another way to do the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation of setEntry
:
void setEntry(int row, int col, double v) {
assert((row >= 0) && (row < dimension));
assert((col >= 0) && (col < dimension));
_m2storage[index(row, col)] = v;
}
int index(int row, int col) => (col * 2) + row;
Implementation of r0c0
:
set r0c0(double value) => _m2storage[0] = value;
I believe r0c0
is less verbose than setEntry
and it will be more performant than setEntry
.
The addition of these setters depends on if we consider performance important. According to @rakudrama "writers of math code often are concerned with performance.".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the index
would be calculated directly inside of setEntry
instead of being a separate method I doubt that this would give any noticeable performance benefits (might not even be noticable with the extra method call either). But it would be very interesting to see a benchmark!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion @spydon!
So r0c0 = 1
is faster than setEntry(0, 0, 1)
.
Results varied from time to time but r0c0
was always faster.
I'm running using MacBook Pro M1 (macOS 12.2 21D49 darwin-arm).
Hope this helps.
Code
The below snippets show the code used to derive the above table.
final stopwatch = Stopwatch()..start();
for (var i = 0; i < 100; i++) {
Matrix2.zero().setEntry(0, 0, 1);
}
stopwatch.stop();
print(stopwatch.elapsedMicroseconds);
final stopwatch = Stopwatch()..start();
for (var i = 0; i < 100; i++) {
Matrix2.zero().r0c0 = 1;
}
stopwatch.stop();
print(stopwatch.elapsedMicroseconds);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the benchmark, have you moved in the index calculation to be inside of setEntry
so that it doesn't have to do that unnecessary extra method call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the benchmark is with the code currently provided in this PR.
I'm intrigued to know the difference without the index call. I will look into it soonish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the recent inactivity, I've been out of the office. My plan is to look into this in the upcoming week soon.
Hi, Is this pull request still active? |
Hi @johnmccutchan ! I stopped developing it due to lack of contribution feedback. I can have a look at it and finish what is required here if the feature is willing to be accepted. |
Please re-open if you refresh your patch. |
STATUS: Draft
Description
Adds setters
r0c0
,r0c1
,r1c0
andr1c1
toMatrix2
,Matrix3
andMatrix4
.Why?
Sometimes it is very inconvenient and verbose to create Matrices.
The value setters aim to improve the developer experience.
Additional Context
#265
#265 (comment)
#265 (comment)