fix:don't assume that arma::eig_sym() always succeeds #2088
Conversation
There was a problem hiding this comment.
Hey @jeffin143, thanks for handling this! Just a couple comments, let me know if I can clarify them further. :)
|
Hey @jeffin143, just wanted to see if you had had a chance to look into this one. I think it's really close and we could have it merged quickly and resolve #1840. :) |
|
@rcurtin , slightly busy with Christmas seasons and new year :) , will take a look by 5th Jan and close the issue |
|
@jeffin143 no worries, enjoy the Christmas season. Just wanted to make sure it hadn't fallen off your list or anything. :) |
|
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍 |
|
Let's keep the issue open. |
|
This is ready to be reviewed, Let's get this off the list |
| covariance = arma::symmatu(covariance); | ||
| if (!arma::eig_sym(eigenvalues, eigenvectors, covariance)) | ||
| { | ||
| Log::Warn << "applying to constraint could not be accomplished." |
There was a problem hiding this comment.
If arma::eig_sym() fails, eigval is reset so that it has no elements -- see the eig_sym() documentation.
So I don't think we can recover from that, I guess instead we could throw a std::runtime_error?
There was a problem hiding this comment.
Yeah, I would agree with that. The same is true for all the other situations too, in my opinion. You could use Log::Fatal too, that throws a std::runtime_error; or you can directly throw a std::runtime_error---personally I don't have a preference which.
There was a problem hiding this comment.
The issue have been resolved, changed all occurrences of Log::warn with Log::fatal
There was a problem hiding this comment.
@jeffin143 thanks for working on it---I agree, let's get it off the list. :)
| covariance = arma::symmatu(covariance); | ||
| if (!arma::eig_sym(eigenvalues, eigenvectors, covariance)) | ||
| { | ||
| Log::Warn << "applying to constraint could not be accomplished." |
There was a problem hiding this comment.
Yeah, I would agree with that. The same is true for all the other situations too, in my opinion. You could use Log::Fatal too, that throws a std::runtime_error; or you can directly throw a std::runtime_error---personally I don't have a preference which.
![mlpack-bot[bot]](https://avatars.githubusercontent.com/in/23934?s=60&v=4){kind=small}
There was a problem hiding this comment.
Awesome, great to have this solved! Thank you @jeffin143. 👍
Handling : #1840
@conradsnicta - Let me know if I am missed something.