-
Notifications
You must be signed in to change notification settings - Fork 767
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
noise models: Add getters for all model parameters #1175
Conversation
PS: I didn't add a new virtual method
|
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.
I think this is OK and I'll approve it.
If time/resources were not an issue, I'd probably would have preferred a more "meaningful" name for each parameter, and - while we're at it -- better docs for each model (e.g., a doxygen block with the formula and a name for the parameter).
But, unless you have the time (or a student with time) I think this a good compromise.
Signed-off-by: Jose Luis Blanco Claraco <joseluisblancoc@gmail.com>
Signed-off-by: Jose Luis Blanco Claraco <joseluisblancoc@gmail.com>
@dellaert I know... I also felt the same about the docs but didn't want to extend the scope of the PR too far away :-) While documenting and going through the derivatives, I think I fixed a wrong "x 2" factor in the DCS weight, here, with the equations justifying the change here: gtsam/gtsam/linear/LossFunctions.h Lines 377 to 389 in 017e3cd
Please, confirm whether it's right now or not... I think the "2" should go on the second line, otherwise it would become a "4". Not a big deal probably anyhow... |
This reverts commit 42b795c.
Ok, I just reverted the last "fix" of the DCS equation to make unit tests happy. If you think it was a real bug, we might open a issue ticket and handle it in a separate PR. |
OK, I'll merge this. |
Add missing "getters" for noise models. Otherwise, it's impossible to find out their internal parameters after construction time.
They were needed to allow the development of this new non-intrusive alternative serialization library.