Skip to content
Snippets Groups Projects

Motion buffer corner case

Merged Jeremie Deray requested to merge motion_buffer_corner_case into master

That's the outcome of a discussion between @jsola and Elena (intern at pal). It seems that if the motion buffer size is less than 2 then the covariance matrix is singular, hence the need for a little perturbation.

Merge request reports

Merged by avatar (May 17, 2025 7:18pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Developer

    @jsola Can you please review, since I wasn't there I'm not 100% sure to grasp the whole issue/fix.

  • Jeremie Deray added 1 commit

    added 1 commit

    • 8a15ccd3 - avoid singular matrix from small motion buffer

    Compare with previous version

  • I cannot merge this PR for the following reason:

    The constant 0.0001 in line std::abs(0.0001*_data(0))*Eigen::MatrixXs::Identity(delta_cov_size_,delta_cov_size_); is hardcoded. Instead, this value should be read from a YAML file, via one over these two options:

    1. a sigma value specific of the sensor, in SensorOdom2DParams.yaml, which is transferred to the processor at (1.a) installation time (see Problem.installProcessor()) or at (1.b) construction time.
    2. a sigma value specific of the processor, in ProcessorOdom2DParams.yaml.

    The sigma values are then used as in e.g.

    non_meas_noise_var = non_meas_noise_std^2 // this can be done at construction time std::abs(non_meas_noise_var*_data(0))*Eigen::MatrixXs::Identity(delta_cov_size_,delta_cov_size_);

    We've had already several issues with hardcoded covariances matrix in wolf. Normally, they have been put there to avoid singularity in a number of situations. But it's never a good idea to have hardcoded values because when the application changes they might be not well suited.

    Please see if you can improve on this. A new comment here and I will have it a second look.

    Edited by Joan Solà Ortega
  • Author Developer

    Thanks for the review.
    No problem to find a non-hard-coded way :thumbsup: .
    Again, this was a fix from Elena, could you please explain me why is this necessary in a few words ?
    If a few words isn't enough then next time we meet ;)

  • Next week at iri? See if Elena can give you an explanation, i think she got it quite right when i explained it to her. Hilario was there too.

  • Author Developer

    The problem is that she finished her internship before I got back x).

    Next week at iri!

  • I am checking the options to fix this MR

    @artivis , is this MR associated to files you did not submit to wolf for any reason? I have to change the API for the ProcessorOdom2D class to add an extra parameter.

  • Author Developer

    Nop this PR is as is.

  • added 1 commit

    • 6c60b6e6 - Add extra perturbation parameter

    Compare with previous version

Please register or sign in to reply
Loading