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

[WIP] SimpleMatrix with initializer_list constructor #1250

Merged

Conversation

nnormand
Copy link
Member

@nnormand nnormand commented Mar 9, 2017

This PR adds a constructor for SimpleMatrix with an initializer_list argument. This is work in progress: the cofactors are NOT computed and it is unclear if they should or not (setComponent doesn't).

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@dcoeurjo dcoeurjo self-requested a review March 31, 2017 12:17
@dcoeurjo dcoeurjo self-assigned this Apr 8, 2017
@dcoeurjo
Copy link
Member

hi @nnormand , I'm on it. Could you please add a test in testSimpleMatrix ?

@dcoeurjo
Copy link
Member

dcoeurjo commented May 13, 2017

I agree.. cofactor must be updated before returning the determinant or "cofactor" methods.

@dcoeurjo
Copy link
Member

dcoeurjo commented Jul 3, 2017

fine. can someone review this PR ?

@JacquesOlivierLachaud
Copy link
Member

I will try to review it this week. One remark: it would be nice to have a non-invasive editor, so that unrelated files are not modified. Here there are 22 files changed, whereas certainly one or two are concerned.

@dcoeurjo
Copy link
Member

dcoeurjo commented Jul 4, 2017

This PR also contains the new .clang-format rules in a previous PR. My push uses this rules to auto-format the modified file. I don't understand why other files have been processed BTW

@dcoeurjo
Copy link
Member

dcoeurjo commented Jul 4, 2017

you only have to review SimpleMatrix constructor.

@JacquesOlivierLachaud
Copy link
Member

Ok. No problem. I will try to review it tonight.

@JacquesOlivierLachaud
Copy link
Member

I only looked at SimpleMatrix.* and testSimpleMatrix.cpp. It is fine with me. My only remark is to add in the initialiser_list constructor the fact that matrices are filled in row by row from top left to top right then again to bottom.

@dcoeurjo
Copy link
Member

dcoeurjo commented Jul 6, 2017

agree

@dcoeurjo
Copy link
Member

dcoeurjo commented Jul 7, 2017

I've updated the doc of the constructor. Fine for you ?

@JacquesOlivierLachaud
Copy link
Member

It is fine with me. I've restarted the travis job that fails because of excess time.
I merge afterwards.

@JacquesOlivierLachaud JacquesOlivierLachaud merged commit 1d4f575 into DGtal-team:master Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants