-
Notifications
You must be signed in to change notification settings - Fork 118
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
nonlinz.cpp: use eigen instead of sparse13 #2623
Conversation
✔️ f71b138 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
817edb2
to
c2aa51a
Compare
✔️ ba6cb29 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
✔️ 62b1507 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✔️ 01a2dfc -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
✔️ 12b8d64 -> Azure artifacts URL |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2623 +/- ##
=======================================
Coverage 66.18% 66.19%
=======================================
Files 559 559
Lines 108940 108880 -60
=======================================
- Hits 72107 72070 -37
+ Misses 36833 36810 -23 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
✔️ beaacfa -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✔️ 5130833 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
LGTM in general and has reasonable changes. Some small suggestions only. I must say that I'm not familiar with sparse13
and Eigen
though. For the validity of the changes I would refer to the modeldbci
✔️ bc5c51c -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
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.
Overall really good changes, but I do have a few questions and requests :)
✔️ 27af30a -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b7c5cb8
to
7312380
Compare
This comment has been minimized.
This comment has been minimized.
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
✔️ 5f3bf6d -> Azure artifacts URL |
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.
looks good!
diag_
have been removed. Those was pointer for direct access to the diagonal elements, butEigen
can relocate matrix and invalidate pointers.double* rv_
(realv_
) anddouble* jv_
(imagv_
), have been changed tostd::vector<double> v_
.char* m_
(the sparse13 matrix) have been changed to aEigen::SparseMatrix<std::complex<double>> m_
. The solverEigen::SparseLU lu_
have been added. One of the main difference is thatsparse13
is1-indexed
andEigen
:0-indexed
.complex
version ofsparse13
have been removed because this was the last use.transfer_amp
,input_amp
,transfer_phase
,input_phase
andratio_amp
have all been simplified by using standard functions ofstd::complex
instead of computing by ourself.v_index
have been removed because this was a vectori -> i + 1
. Now thatEigen
is0-indexed
it becomes really useless.partrans.cpp
the functionpargag_jacob_rhs
has been modified to supportstd::complex<double>
, so know we do only one call of the function (before one with real and one with complex part), but inside we loop twice on real and complex because the algorithm is hard to understand to do everything in once.