Fixing bugs in the exterior algebra Gröbner basis F4 algorithm implementation#41596
Fixing bugs in the exterior algebra Gröbner basis F4 algorithm implementation#41596vbraun merged 2 commits intosagemath:developfrom
Conversation
|
@fchapoton Can you review this? I know this is a bit outside your usual expertise, but I'm happy to help explain stuff. This is a fairly serious bug silently returning wrong answers, so I want to get it reviewed and merged quickly. |
|
Documentation preview for this PR (built with commit 8a1fdc1; changes) is ready! 🎉 |
|
I can try, but maybe not in depth. You introduced a TAB in the reference file, near Stokes, I think |
|
Thank you. Fixed. |
|
Some stupid questions:
I am afraid that I will not be able to check the math, sorry. Given that the tests pass, I am ready to give a positive review based on trust and my superficial reading. Would it be enough for you ? |
|
The leading monomial/support/coefficient is the usual term from Groebner theory, where a particular term ordering is taken (here only degrevlex, deglex, neglex are implemented). The leading support is stored in Honestly, I don't expect someone to fully go through the math as I had to do some for this as it doesn't appear to be in the literature in this form AFAIK. The implementation mostly uses the ideas from the classical F4 algorithm taken with making sure it expands the GB by things that remove the leading term (which doesn't happen in the classical theory). If this and the passing doctests are sufficient for you, then let's get this in as this now covers cases that were broken before. |
fchapoton
left a comment
There was a problem hiding this comment.
ok, looks good enough and the tests pass locally
sagemathgh-41596: Fixing bugs in the exterior algebra Gröbner basis F4 algorithm implementation <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This fixes serious bugs in the computation of the Gröbner basis of exterior algebra: - not doing all proper reductions (thanks to Michael Cuntz to pointing out); - wrong type of Gröbner basis (as defined by Stokes), which reduces uniquely but does not work for ideal containment; - modifying internal states of elements during specific reduction steps (i.e., they are not acting like immutable (hence, hashable) elements). We rework the implementation to address these (and hopefully some optimizations). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#41596 Reported by: Travis Scrimshaw Reviewer(s): Frédéric Chapoton
|
Thank you very much. |
|
I wonder in which dimensions |
|
That's a good question. By brute-force-linear-algebra, you mean construct a basis in echelon form for the whole ideal, correct? My guess is something like |
This depends on the field you are a lot. |
This fixes serious bugs in the computation of the Gröbner basis of exterior algebra:
We rework the implementation to address these (and hopefully some optimizations).
📝 Checklist
⌛ Dependencies