fix: use per-estimator degrees of freedom in MAGSAC scoring#50
Draft
f-dy wants to merge 1 commit into
Draft
Conversation
7a0aacc to
0faa649
Compare
0041613 to
d0459ba
Compare
The MAGSACScoringFunction hardcoded DoF=4 (appropriate for Sampson distance on 2D-2D correspondences) for all estimator types. This is incorrect for: - Linear model (plane/line): scalar distance -> chi^2(1) - Homography/PnP/radial homography: 2D transfer error -> chi^2(2) - Rigid transformation: 3D Euclidean distance -> chi^2(3) Fix: - Add static constexpr getDegreesOfFreedom() to each estimator - Add precomputed E1(x) table for DoF=1 (gamma_values_dof1.cpp) - Use closed-form expressions for DoF=2 (erfc) and DoF=3 (exp) - Use existing stored_gamma_values table for DoF=4 (unchanged) - Dispatch via if constexpr in the scoring loop Backward compatible: DoF=4 path uses the exact same table as before.
f-dy
added a commit
to f-dy/graph-cut-ransac
that referenced
this pull request
Jun 1, 2026
…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.
Summary
The
MAGSACScoringFunctioninscoring_function.hhardcodes_DimensionNumber = 4for all estimator types. This is only correct for the Sampson distance used by fundamental/essential matrix estimation (where the residual is derived from 4D data(x1,y1,x2,y2)and(r/σ)² ~ χ²(4)).For other estimators, the degrees of freedom of the squared residual are different:
dx²+dy²du²+dv²dx²+dy²+dz²The wrong DoF propagates to:
√χ²₀.₉₉(2) = 3.03vs the hardcoded3.64 = √χ²₀.₉₉(4)Γ(1.5, x)= DoF=4 only)The practical effect is that MAGSAC weights decay with the wrong tail shape — χ²(4) has a fatter tail than χ²(1) or χ²(2), so the scoring is more permissive of outliers than the statistical model warrants.
Fix
Add
static constexpr size_t getDegreesOfFreedom()to each estimator, returning the correct value for its residual type.Add precomputed E₁(x) table (
gamma_values_dof1.cpp) for DoF=1, since E₁ has no closed-form expression in standard C++.Use closed-form expressions for DoF=2 and DoF=3:
Γ(0.5, x) = √π · erfc(√x)viastd::erfcΓ(1, x) = e⁻ˣviastd::expUse existing
stored_gamma_values[]table for DoF=4 (unchanged).Dispatch via
if constexprin the scoring loop, with astatic_assertin theelsebranch to catch unsupported DoF values at compile time.Backward compatibility
stored_gamma_values[]table — results are numerically identical.gamma_values.cppfile is still included and used for DoF=4.<cmath>functions (erfc,exp,sqrt).Verification
Split out: null-pointer crash in
solver_linear_model.hThe null-
sample_crash guard that was originally bundled here has beenextracted to its own standalone PR, #52
(it has zero coupling to the gamma/DoF work and can merge independently).
DoF=1 weight singularity (fixed)
The E₁(x) table for DoF=1 has E₁(0)=∞ at index 0. When a small residual rounds to
index 0, the lookup returned ∞ → infinite weight. Fixed by capping
table[0] = E₁(step)(first finite entry) in
gamma_values_dof1.cpp.Known gamma-weight issues (NOT fixed here — flagged for maintainers)
Reviewing
MAGSACScoringFunctioninscoring_function.hagainst the MAGSAC++ paper(Eq. 2.1) revealed two pre-existing inaccuracies, also present in magsac's IRLS:
Γ(a, i/1000)(step 1/1000) but is indexed withprecision_of_stored_gammas = 1e4→ the gamma is evaluated at 10× the intended argument.σ_max = increasedThresholddirectly; the paper usesσ_max = threshold / k.10/k²≈ 0.75 for n=4). Fixing B2 alone (precision = 1e3) is harmful — it removes the cancellation (argument becomes1/k²of correct). Both must be fixed together. Not done here: changes results for all models and needs accuracy benchmarks.