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

Added a method rowAdd(...) to Matrix #138

Merged
merged 9 commits into from
Mar 4, 2019

Conversation

christianbender
Copy link
Contributor

Issue #64
I added the feature as requested:

Adds a multiple of one row to another inside a matrix.

rowAdd(dest : number, source : number, scalar : number = 1.0 ) : Matrix

Inclusive tests

@mateogianolio
Copy link
Owner

Thanks, will have a look at it this weekend.

@christianbender
Copy link
Contributor Author

christianbender commented Feb 28, 2019

@mateogianolio Super, Thanks.

Copy link
Owner

@mateogianolio mateogianolio left a comment

Choose a reason for hiding this comment

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

Some notes:

  • Don't create a new array unless you really have to (and if you do, use typed arrays). Array instantiation is expensive, especially when using push() (needs to reallocate memory every time it's called).
  • You have two for-loops that loop through the same memory space. They should be combined into one.

See this benchmark: https://jsbench.me/1bjssxmc69/1

src/Matrix.prototype.spec.ts Outdated Show resolved Hide resolved
src/Matrix.ts Outdated Show resolved Hide resolved
@christianbender
Copy link
Contributor Author

@mateogianolio

  • I changed the method rowAdd(...) as requested and made it more effiecient.
  • In addition I removed the runtime type checks and changed the test-suite accordingly. Removed the exception-test

@christianbender
Copy link
Contributor Author

@mateogianolio Done. I added the preconditions again

@codeclimate
Copy link

codeclimate bot commented Mar 4, 2019

Code Climate has analyzed commit d3faae3 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 90.6% (-3.2% change).

View more on Code Climate.

@mateogianolio mateogianolio merged commit d3faae3 into mateogianolio:master Mar 4, 2019
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

2 participants