-
Notifications
You must be signed in to change notification settings - Fork 795
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
Enable GNC Optimizer #1572
Enable GNC Optimizer #1572
Conversation
…/stl.h is enabled" This reverts commit 64c2850.
@varunagrawal : this looks excellent. I'll leave the question of (re)naming of GncHelper to @dellaert . My only comment/suggestion would be to add a few more unit tests. This is a quite basic functionality and it we get it wrong, it might be very hard to debug in downstream applications. |
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.
Awesome!. I'm fine with the location of the file. Agreed with Luca on unit tests, but my main request is to add some information in GncHelpers as to where this code comes from? Is this de-novo code? Was it copied from boost? Also, if it only implements chi-square, why not name it as such? Finally, this is not a class I think so the file should start with a lowercase letter.
I looked at various implementations from both Boost and GCEM to understand how Halley's algorithm worked and how to deal with different datatypes effectively, but I wrote this pretty much on my own. After some thought, I am considering these changes:
|
Sounds good. Please do document the above. Having straight copies of code from boost where possible would be preferable as those functions would not have to be unit-tested, and it is acceptable under their license. |
...with attribution, of course |
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.
Thanks !!!
I wanted to give a quick update on this:
|
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!
…n easy packaged version
Cephes license can be found here: https://smath.com/en-US/view/CephesMathLibrary/license |
Added our own implementation of Halley's Iteration to solve for the chi square inverse function. This allows us to use theChi2inv
function without relying on Boost, thus enabling the use of GNC as an optimizer again.I need some feedback on the following points:1. I have named the file with Halley's iteration asGncHelpers.h
. Should I rename this to something else that is more generic? Most of the code is for computing the gamma distribution inverse, which is used to compute the chi square inverse.2. Should the above file be withinnonlinear/internal
? Currently it is innonlinear
.3. Any documentation or other aspects that would be needed? I don't expect this code to be user facing and I would welcome replacing this function with another library that is better tested and documented.Implement
chi_squared_quantile
(aka thechi2inv
function) using the incomplete gamma inverse function fromcephes
.I added
cephes
to3rdparty
and added the necessary CMake files to build and link against GTSAM. This makes the change for us really simple.