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

fix the bug for threshold update #148

Merged
merged 5 commits into from
Dec 13, 2024
Merged

fix the bug for threshold update #148

merged 5 commits into from
Dec 13, 2024

Conversation

lpanaf
Copy link
Collaborator

@lpanaf lpanaf commented Dec 12, 2024

fix the bug that the update of threshold from command line is not reflected in the structure

@lpanaf lpanaf requested a review from ahojnnes December 12, 2024 14:31
@@ -19,9 +19,13 @@ struct BundleAdjusterOptions : public OptimizationBaseOptions {
// Constrain the minimum number of views per track
int min_num_view_per_track = 3;

void UpdateThreshold() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this fix, I suggest the following:

struct BundleAdjusterOptions {
  double thres_loss_function = 1;

  std::shared_ptr<ceres::LossFunction> CreateLossFunction() {
    return std::make_shared<ceres::HuberLoss>(thres_loss_function);
  }
};

Since you explicitly have to call CreateLossFunction(), it is harder to accidentally forget calling the UpdateThreshold().

@ahojnnes
Copy link
Contributor

The proposal was to return the shared pointer and not assign it. In the respective BA and GO you would then call this method to create the loss function (always with the latest threshold).

@lpanaf lpanaf merged commit ebd50af into main Dec 13, 2024
7 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.

2 participants