fix: correct MAGSAC++ IRLS gamma weight (B2 + B3) in getScore#53
Draft
f-dy wants to merge 1 commit into
Draft
Conversation
…oF=4 tables getScore computed the per-point MAGSAC++ weight at the wrong gamma argument: - B3: sigma_max was increasedThreshold (the inlier threshold) rather than threshold/k. Now sigma_max = increasedThreshold/k drives both the argument and the prefactor; the inlier-collection cutoff stays at increasedThreshold = k*sigma_max. - B2: the index used the 1e4 precision against the coarse stored_gamma_values table (step 1e-3), reading the gamma at 10x the argument. Now it indexes the fine stored_complete_gamma_values table (step 1e-4) whose spacing matches 1e4. Tables (gamma_values.cpp) are now generated by scripts/generate_gamma_values.py (scipy.special) and trimmed to the useful interval [0, k^2/2]: 66385 entries (was [0,10]=100001). The dead coarse table is removed. The generated C++ documents how it was produced and why the length; the table is sized for the precise k = sqrt(chi2_0.99(4)) so it works with either the imprecise k literal kept here (3.64) or the precise per-DoF k introduced by danini#50. k stays at the existing imprecise literal in this PR (focused on B2/B3); precise per-DoF k is danini#50's job. Changes results for existing DoF=4 estimators: validate on Adelaide/EVD/homogr before submission.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: correct MAGSAC++ IRLS gamma weight (B2 + B3) in
getScoreProblem
MAGSACScoringFunction::getScore(scoring_function.h) computes the per-pointMAGSAC++ weight at the wrong gamma argument, due to two compounding bugs:
sigma_max = increasedThreshold(theinlier threshold itself). The MAGSAC++ paper — and this repo's own
getModelQualityPlusPlus— usesigma_max = threshold / k, wherek = √χ²₀.₉₉(dof). The function even definesthreshold_to_sigma_multiplier = 1/kbut never applies it.
round(precision_of_stored_gammas · …)withprecision_of_stored_gammas = 1e4,but
stored_gamma_valuesis spaced at 1e-3 ([0, 36], 36 001 entries). Sothe lookup reads
Γ(1.5, 10·t)— the gamma at 10× the intended argument.B2 and B3 partially cancel (net factor
10/k² ≈ 0.75for DoF=4), which is whyMAGSAC++ still works well in practice — but the weight is not the paper's.
Fix
sigma_max = threshold/kdrives both the gamma argument and the prefactor.residual > increasedThreshold) is unchanged— the paper truncates the weight at
r ≤ k·σ_max = threshold.stored_complete_gamma_valuestable, whose1e-4 spacing matches
precision_of_stored_incomplete_gammas.After this,
getScore's weight matchesgetModelQualityPlusPlusand the paper.Tables: generated + trimmed to the useful interval
gamma_values.cppis now produced byscripts/generate_gamma_values.py(scipy.special), which emits the C++ with a header comment documenting how it was
generated and why its length. The two live DoF=4 tables
(
stored_complete_gamma_values= Γ(1.5,·),stored_lower_incomplete_gamma_values= γ(2.5,·)) are trimmed to the useful interval
[0, k²/2]— 66 385 entries,down from
[0, 10]= 100 001 — because the weight is truncated at the inliercutoff
r ≤ k·σ_max, so the argument never exceedsk²/2. The dead coarsetable (
stored_gamma_values/precision_of_stored_gammas/stored_gamma_number) is removed (it had no remaining consumer once getScoremoved to the fine table — verified across the whole gcransac repo).
The table is sized for the precise
k = √χ²₀.₉₉(4)(the largest plausiblecutoff), so it works unchanged whether the consumer uses the imprecise
kliteralor a precise one; lookups clamp to
stored_incomplete_gamma_number.Note on
kprecision (deferred to #50)This PR keeps the existing imprecise
kliteral (k(DoF=4) = 3.64, with itsmatching
gamma_k) to stay focused on the B2/B3 weight bug. Precise per-DoFk = √χ²₀.₉₉(DoF)is introduced by the per-estimator-DoF PR (#50). Because thetable is sized for the precise
k, #50 can switch to precisekwithoutregenerating or resizing the table.
This changes results for the existing (DoF=4) estimators (it removes the
10/k²distortion). It must be validated on the paper's datasets(Adelaide / EVD / homogr: median error and failure rate, before/after)
before merging.
Companion magsac PR fixes the identical bug in
magsac.h::sigmaConsensusPlusPlus:danini/magsac#49 — the two are released together. Independent of the per-estimator-DoF PR (#50),
which is adapted onto this corrected base separately. The now-unused coarse
stored_gamma_values/precision_of_stored_gammascan be retired once #50'sadaptation also stops using them.