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

Remove bad observations #319

Merged
merged 19 commits into from
Oct 14, 2021
Merged

Remove bad observations #319

merged 19 commits into from
Oct 14, 2021

Conversation

patripfr
Copy link
Contributor

@patripfr patripfr commented Jun 16, 2021

Added a new command remove_invalid_observations -> rio, that solves the issues brought up by check_map_consistency, namely landmarks that due to loop closure are observed multiple times from the same frame (which is impossible). This command splits away the worse tracks and reinitializes them again as new landmarks.

@patripfr patripfr requested a review from smauq June 16, 2021 17:42
Copy link
Member

@smauq smauq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very general comments, and a few smaller ones I just noticed. If you do some of them I'm happy to have a second look

@smauq
Copy link
Member

smauq commented Jul 3, 2021

@patripfr so I should have solved now all the issues we talked about (parallelizing removal of tracks and handling -1 track ids). If you could check my code and I think there's a few last points:

  • Some simple unit test maybe
  • Initialize landmarks and triangulate only for affected vertices/landmarks (this one you also have to check if it makes sense from a speed perspective)
  • If you could for curiosity run a trajectory quality evaluation on EuRoC between using and not using the new elq it would be super cool to know if it improves trajectory quality at all

@smauq smauq changed the title Feature/remove bad observations Remove bad observations Oct 10, 2021
Copy link
Member

@smauq smauq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor changes and otherwise LGTM. I'll do these myself and then I think it's in a good state to merge

@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 10, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 12, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 12, 2021
@ethzasl-jenkins
Copy link

Test PASSed.

@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 14, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 14, 2021
@ethz-asl ethz-asl deleted a comment from ethzasl-jenkins Oct 14, 2021
@ethzasl-jenkins
Copy link

Test FAILed.

@ethzasl-jenkins
Copy link

Test PASSed.

@smauq smauq merged commit 97eb7cf into develop Oct 14, 2021
@smauq smauq deleted the feature/remove_bad_observations branch October 14, 2021 23:00
@ethzasl-jenkins
Copy link

Test PASSed.

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.

3 participants