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

Updated Map Matching With TEBLID and CLAHE #797

Merged
merged 63 commits into from
Jun 29, 2024

Conversation

rsoussan
Copy link
Member

Added TEBLID feature support for mapping and localization, added option for using CLAHE for histogram equalization, added options for adding similar images, best previous image, and essential matrix filtering.

Copy link
Contributor

@trey0 trey0 left a comment

Choose a reason for hiding this comment

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

Hi, Ryan- Not sure I should presume to approve because I'm pretty out of the loop on some of the integration issues. This is looking great so far! Just a few questions / minor comments.

verbose_localization = false
visualize_localization_matches = false

-- BRISK
Copy link
Contributor

Choose a reason for hiding this comment

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

Are BRISK and TEBLID the only choices? What about SURF? What about TEBLID256 vs. TEBLID512? (It's ok if this file isn't merged in its final form because we can tune it after the ISAAC16 code freeze. But it might help to list the available feature types at the top of the file even if we don't define parameters for all the types yet. Also, not sure about this, but it might make sense to define a default value for each parameter that applies across all feature types while enabling a feature-type-specific value to override that.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added teblid512 and teblid256, we don't really support SURF for loc since we never merged the vocab branch so I'll leave that out

* @copyright 2021 Xoan Iago Suarez Canosa. All rights reserved.
* Constact: [email protected]
* Software developed in the PhD: Low-level vision for resource-limited devices
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

(Used similar link with license as suggested by @bcoltin)

if (detector_name == "ORGBRISK") {
prefix = "brisk_";
} else if (detector_name == "TEBLID") {
prefix = "teblid_";
Copy link
Contributor

Choose a reason for hiding this comment

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

(Possibly support "TEBLID512" and "TEBLID256"?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

histogram_equalization);
loc_params.histogram_equalization);
// Check consistency between clahe params
if (loc_params.use_clahe && (loc_params.histogram_equalization != 3 || map_->GetHistogramEqualization() != 3)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The literal 3 here is a bit obscure. Probably there should be an enum or at least a macro that you can compare to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -272,15 +229,12 @@ struct SparseMap {
std::vector<cv::Mat> cid_to_descriptor_map_;
// generated on load
std::vector<std::map<int, int> > cid_fid_to_pid_;
std::vector<std::map<int, int>> cid_to_matching_cid_counts_;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're adding new fields to the map, ideally we should also log their memory usage in our memory debug statistics. (Not sure if you were aware there's per-field code for that.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh actually I wasn't aware of that, which file(s) should I edit?

Copy link
Member Author

Choose a reason for hiding this comment

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

(found it, looks like it's still in PR, I'll update the pr)

@rsoussan rsoussan marked this pull request as ready for review June 28, 2024 21:28
Copy link
Member

@bcoltin bcoltin left a comment

Choose a reason for hiding this comment

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

Fix headers like we discussed and any minor things and if you've tested looks good.

/**
* @copyright 2021 Xoan Iago Suarez Canosa. All rights reserved.
* Constact: [email protected]
* Software developed in the PhD: Low-level vision for resource-limited devices
Copy link
Member

Choose a reason for hiding this comment

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

what license?

Copy link
Member

Choose a reason for hiding this comment

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

and does this really need to be included instead of linked to

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@rsoussan rsoussan merged commit 2714fcb into nasa:develop Jun 29, 2024
4 checks 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.

4 participants