Repository navigation
Conversation
Contributor
Author
|
@ruevs Could you review this proposed fix for #1723 when you have time? Both reported pairs are now detected as redundant on the first solve in the local tests, including rotated/scaled copies. I'd particularly appreciate your feedback on the numerical cutoffs and the cost of the SVD fallback before this is merged. |
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.
Adding a parallel constraint to r00c/r010 in #1723 is accepted as independent on the first solve (2 DoF), then reported as redundant on the next solve (3 DoF). Nearly dependent Jacobians can be reported as full row rank by SparseQR's fill-reducing ordering, and the result changes after a small Newton step.
Keep SparseQR as the normal rank check. When it reports full row rank but its smallest R pivot is below
1e-6times the largest, verify the rank with Eigen's Jacobi SVD. Normalize rows and columns of a copy first, then use a relative singular-value cutoff of1e-9. The Newton matrix and least-squares step are unchanged; already rank-deficient QR results keep their existing path.The verification detects both reported pairs immediately and keeps 3 DoF across repeated solves, without increasing SparseQR's threshold for every sketch.
Regression coverage includes the reporter's model, both pairs, 10 repeated solves, four rotations at three scales with translation, and small independent-column checks. Both model regression tests fail on the original rank check. The full headless Debug suite passes with sanitizers and OpenMP: 303 cases, 1,461 checks. Local sanitizer validation also includes the separate cleanup fix from #1817 to avoid that known shutdown leak. No interactive GUI testing was performed.
The fallback is more expensive: a synthetic
-O2probe (three repetitions) increased from about 2.3 ms to 22 ms at 128x128, and from 14 ms to 297 ms at 256x256. Well-conditioned cases retain the QR path. The cutoffs and large-matrix cost need review; these tests do not establish correct classification of every near-singular configuration.Fixes #1723.