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

Simple fix to issue #80 #83

Merged
merged 19 commits into from Feb 20, 2019

Conversation

@favre49
Copy link
Member

commented Feb 16, 2019

This is a simple fix to issue #80 . This is done by adding small values to the diagonal of the covariance matrix whenever the Cholesky decomposition fails to make the matrix positive definite. According to my testing, this seems to have fixed the problem.

favre49 added some commits Feb 2, 2019

DE
@rcurtin
Copy link
Member

left a comment

@favre49 thanks for taking the time to work up a fix. My comments are pretty minor; if you can address those, I think we should merge it. 👍 Also, would you like to add a note to HISTORY.md detailing the change? If not I will do it during merge.

* [Differential Evolution - A simple and efficient adaptive scheme for global optimization over continuous spaces](http://www1.icsi.berkeley.edu/~storn/TR-95-012.pdf)
* [Differential Evolution in Wikipedia](https://en.wikipedia.org/wiki/Differential_Evolution)
* [Arbitrary functions](#arbitrary-functions)

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

Oops, I think these changes should not be a part of this PR. :)

arma::mat I(iterate.n_elem, iterate.n_elem, arma::fill::eye);
C.slice(idx0) += I*(1e-16);
covLower = arma::chol(C.slice(idx0), "lower");
}

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

Actually you can simplify this a bit...

C.slice(idx0).diag() += 1e-16;

Also, if 1e-16 is generally sufficient I'm okay with that, but it's also possible to loop iteratively until the Cholesky decomposition succeeds. You could use the version chol(C.slice(idx0), covLower, "lower") which returns a bool, and loop until it's true.

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 20, 2019

Member

Here is a simplification. :)

while (!arma::chol(covLower, C.slice(idx0), "lower"))
  C.slice(idx0).diag() += 1e-16;

This comment has been minimized.

Copy link
@favre49

favre49 Feb 20, 2019

Author Member

Yup, far simpler, thank you for the suggestion

@@ -281,4 +294,4 @@ double CMAES<SelectionPolicyType>::Optimize(

} // namespace ens

#endif
#endif

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2019

Member

No need to remove the newline from the end of the file, please. 👍

favre49 added some commits Feb 19, 2019

Revert "Made the correction a loop"
This reverts commit 833cda9.
@zoq

zoq approved these changes Feb 20, 2019

Copy link
Member

left a comment

Looks good to me as well.

@rcurtin rcurtin merged commit 0dd4d1a into mlpack:master Feb 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@favre49 thanks! 👍

@favre49 favre49 deleted the favre49:issue-#80 branch Feb 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.