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

noise models: Add getters for all model parameters #1175

Merged
merged 4 commits into from
Apr 22, 2022

Conversation

jlblancoc
Copy link
Member

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.

@jlblancoc jlblancoc mentioned this pull request Apr 20, 2022
@jlblancoc
Copy link
Member Author

jlblancoc commented Apr 20, 2022

PS: I didn't add a new virtual method modelParameter() to robust Base for two reasons:

  • ABI break.
  • Not all robust models have 1 parameters ("Null"), and, perhaps, one day somebody creates a model with 2 or N parameters.

Copy link
Member

@dellaert dellaert left a 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 <[email protected]>
Signed-off-by: Jose Luis Blanco Claraco <[email protected]>
@jlblancoc
Copy link
Member Author

@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 :-)
Take a look now, I'm much happier with the docs now, let me know your thoughts, or feel free of merging.

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:

/** DCS implements the Dynamic Covariance Scaling robust error model
* from the paper Robust Map Optimization (Agarwal13icra).
*
* Under the special condition of the parameter c == 1.0 and not
* forcing the output weight s <= 1.0, DCS is similar to Geman-McClure.
*
* This model has a scalar parameter "c" (with "units" of squared error).
*
* - Loss \rho(x) = (c²x² + cx⁴)/(x²+c)² (for any "x")
* - Derivative \phi(x) = 2c²x/(x²+c)²
* - Weight w(x) = \phi(x)/x = 2c²/(x²+c)² if x²>c, 1 otherwise
*/
class GTSAM_EXPORT DCS : public Base {

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.
@jlblancoc
Copy link
Member Author

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.

@dellaert
Copy link
Member

OK, I'll merge this.
I have no time to check if it's a bug or not :-( If you are convinced, let's open an issue to record it at least.

@dellaert dellaert merged commit b1d936e into develop Apr 22, 2022
@dellaert dellaert deleted the feature/add-missing-getters branch April 23, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants