Fix qfsolve #41012
Fix qfsolve #41012
Conversation
user202729
force-pushed
the
fix-qfsolve
branch
from
bee7cad to
8c3442a
Compare
|
Documentation preview for this PR (built with commit 8c3442a; changes) is ready! 🎉 |
| if i >= d: | ||
| # Restrict the quadratic form to a subspace complement to x, then solve recursively | ||
| # note that complementary_subform_to_vector returns the orthogonal (not necessary complement) | ||
| # subspace with respect to self, which is not what we want | ||
| i = next(i for i in range(d) if x[i]) | ||
| from sage.quadratic_forms.quadratic_form import QuadraticForm | ||
| x = QuadraticForm(self.matrix().delete_rows([0]).delete_columns([0])).solve(c) | ||
| return vector([*x[:i], 0, *x[i:]]) |
There was a problem hiding this comment.
I'm not sure I understand this condition here and why it's relevant now and it wasn't here before.
There was a problem hiding this comment.
it has always been relevant, the previous code is just buggy to not include it (to be more precise, this branch is guaranteed to be not hit if the quadratic form is non-degenerate). Try running the code with the fix above (t_MAT) but not the one here and you'll see the IndexError caused by i >= d.
There was a problem hiding this comment.
Yeah I have just tried on some example. Thanks for catching that.
| ... | ||
| ArithmeticError: no solution found (local obstruction at some prime factor of det(self.matrix())) | ||
| """ | ||
| def check_obstruction(x): |
There was a problem hiding this comment.
Do you want this to be a publicly-documented function or just an internal one? If internal, rename to _check_obstruction.
There was a problem hiding this comment.
functions local to another function's body is inaccessible outside anyway, doesn't matter.
There was a problem hiding this comment.
Ah true I didn't notice it was internal, my bad
| N = Matrix(self.base_ring(), d+1, d+1) | ||
| N = matrix(self.base_ring(), d+1, d+1) |
There was a problem hiding this comment.
What's the difference between the two constructors?
There was a problem hiding this comment.
essentially no difference, but since now Matrix is imported as a different entity (the type), I give the constructor the other name. You can't call the type like how you call the matrix constructor.
2 participants
see the newly added doctest, they previously fail.
this is a combination of two bugs
📝 Checklist
⌛ Dependencies