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

Fix for non-symmetric matrix diagonal fill (Closes #619) #622

Merged
merged 1 commit into from Dec 31, 2016
Merged

Fix for non-symmetric matrix diagonal fill (Closes #619) #622

merged 1 commit into from Dec 31, 2016

Conversation

coatless
Copy link
Contributor

Fix for the non-symmetric case of the diagonal fill.

Adds unit tests to the diagonal fill feature as well.

Per discussion in #619, this PR moves the matrix diagonal fill away from using an iterator solution to using the matrix subset operator.

@codecov-io
Copy link

Current coverage is 76.47% (diff: 100%)

Merging #622 into master will not change coverage

@@             master       #622   diff @@
==========================================
  Files            68         68          
  Lines          4004       4004          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           3062       3062          
  Misses          942        942          
  Partials          0          0          

Powered by Codecov. Last update 2883ef3...8aa14aa


R_xlen_t bounds = ( Matrix::nrow() < Matrix::ncol() ) ?
Matrix::nrow() : Matrix::ncol();

Copy link
Member

Choose a reason for hiding this comment

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

I was just squinting at this (and mostly wondering if I had written it on one line) when I realized that

 R_xlen_t bounds = std::min( Matrix::nrow(), Matrix::ncol() );

is probably even easier :) (Subjec to testing in whether stdLLmin() likes the R type etc pp)

Copy link
Member

Choose a reason for hiding this comment

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

I guess one could argue that the ternary operator may be lighter-weight that a function call (which, I bet, ends up inlined). I use that operator a fair amount for alternates too. It just seemed more compact...

@eddelbuettel eddelbuettel merged commit 8e5a3e8 into RcppCore:master Dec 31, 2016
eddelbuettel added a commit that referenced this pull request Dec 31, 2016
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