Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

alestiago
Copy link

STATUS: Draft

Description

Adds setters r0c0, r0c1, r1c0 and r1c1 to Matrix2, Matrix3 and Matrix4.

final m = Matrix2.zero()..r0c0 = 2;

/*
[ 2, 0 ]
[ 0, 0 ]
*/

Why?

Sometimes it is very inconvenient and verbose to create Matrices.

The value setters aim to improve the developer experience.

// Before:
Matrix4(2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);

/// After:
Matrix4.zero()..r0c0 = 2;

Additional Context

#265
#265 (comment)
#265 (comment)

@@ -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;
Copy link
Author

@alestiago alestiago Jun 8, 2022

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 )

Copy link
Collaborator

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?

Copy link
Author

@alestiago alestiago Jun 8, 2022

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

Copy link
Collaborator

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!

Copy link
Author

@alestiago alestiago Jun 8, 2022

Choose a reason for hiding this comment

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

Good suggestion @spydon!

Here is what I got:
perf

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

Copy link
Collaborator

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?

Copy link
Author

@alestiago alestiago Jun 8, 2022

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.

Copy link
Author

@alestiago alestiago Jun 22, 2022

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.

@johnmccutchan
Copy link
Collaborator

Hi,

Is this pull request still active?

@alestiago
Copy link
Author

alestiago commented Jun 20, 2023

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.

@johnmccutchan
Copy link
Collaborator

Please re-open if you refresh your patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants