submodule membership testing ( in operator)#41694
Conversation
|
Documentation preview for this PR (built with commit 1259313; changes) is ready! 🎉 |
tscrim
left a comment
There was a problem hiding this comment.
Why are you directly calling Singular rather than going through the ring's implementation? In particular, it could be extended to work where the polynomial ring does not work in Singular but can compute a GB. (I think there are such rings that can be built, but not 100% sure off the top of my head.)
|
The reason I did is that Sage has a clean high-level API for ideals (.groebner_basis(), .reduce()), but I couldn't find an equivalent one for submodules of free modules over polynomial rings. So I ended up going to Singular's std and reduce at the low level. |
tscrim
left a comment
There was a problem hiding this comment.
Ah, sorry. I misread and thought it was acting on polynomials. Okay, that should be fine.
Although I was looking a bit more. It seems like this is duplicating a fair bit of code from the base class's Module_free_ambient._element_constructor_. I'm thinking we should actually modify that code to add the extra case there.
| sage: N = M.submodule([vector([x - y, z]), vector([y*z, x*z])]) | ||
| sage: Q = M.quotient_module(N) | ||
| sage: NQ = Q.submodule([Q([1, x])]) | ||
| sage: NQ([1, x]) |
There was a problem hiding this comment.
Thank you for adding the test, but it would be good to have a more nontrivial example (i.e., not a generator of the submodule).
|
yes i have noticed the duplication and kind of solved it, i have moved the logic to the parent class and moved the tests there as well |
tscrim
left a comment
There was a problem hiding this comment.
Thank you. One last trivial cleanup thing then a positive review.
|
Oh, also, could you squash everything to a single commit and forcepush? (The thing I'm really after is having at least a bit better commit messages, but I figure this is the easiest way.) |
|
i will try my mind simply fails when writing commits :) , sorry i forgot to pull your edits before force pushing but i have fixed it and deleted the unnecessary imports |
678787a to
f7dcc5f
Compare
|
is there anything i should do about the test that failed and kind of a wired thing |
|
You still have an extra blank line added that doesn’t need to be there (as per the diff). You can ignore that failing test as you can see in the log that it is unrelated to your changes. |
|
Thank you. This is now a positive review. (Strictly speaking, it would be better to avoid repeating the doctests. Yes, they are technically different, but since they use the same rings/elements and code path, it becomes effectively the same test. It might also be good to avoid shortcuts like |
|
Thank you for the review and the positive feedback! I appreciate the notes on the redundant doctests and the use of shortcuts. I'll keep those points in mind to ensure cleaner code in my future contributions. |
Remove the groebner_basis_contains check from Submodule_free_ambient
as there is no algorithm to verify element membership.
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
af3b8aa to
1259313
Compare
|
@tscrim i have rebased the code because it was out of sync with the develop branch, could you add the tag back please? |
|
@rashadalsharpini Please don't do such things after a positive review unless there is an absolute need to (i.e., a merge conflict). It can cause trouble with merging (and it means someone needs to look over it again to check an error didn't get introduced). |
sagemathgh-41694: submodule membership testing ( in operator) ### Description Fixes sagemath#41671 — submodule membership testing (`in` operator) incorrectly returns `True` for elements not in the submodule, when the base ring is a quotient of a polynomial ring. ### Changes - Added `_groebner_basis_contains()` to `Submodule_free_ambient` — performs submodule membership testing via Singular's `std` (Groebner basis) and `reduce`. For quotient rings, it lifts generators and the test vector to the covering polynomial ring and augments with ideal relation generators before computing the Groebner basis. Works for submodules over polynomial rings and quotients of polynomial rings. - Added `_element_constructor_()` override to `Submodule_free_ambient` — calls `_groebner_basis_contains()` when `check=True` and the base ring is exact, raising `TypeError` if the element is not in the submodule. ### Checklist - [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. URL: sagemath#41694 Reported by: Rashad Alsharpini Reviewer(s): Rashad Alsharpini, Travis Scrimshaw
sagemathgh-41694: submodule membership testing ( in operator) ### Description Fixes sagemath#41671 — submodule membership testing (`in` operator) incorrectly returns `True` for elements not in the submodule, when the base ring is a quotient of a polynomial ring. ### Changes - Added `_groebner_basis_contains()` to `Submodule_free_ambient` — performs submodule membership testing via Singular's `std` (Groebner basis) and `reduce`. For quotient rings, it lifts generators and the test vector to the covering polynomial ring and augments with ideal relation generators before computing the Groebner basis. Works for submodules over polynomial rings and quotients of polynomial rings. - Added `_element_constructor_()` override to `Submodule_free_ambient` — calls `_groebner_basis_contains()` when `check=True` and the base ring is exact, raising `TypeError` if the element is not in the submodule. ### Checklist - [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. URL: sagemath#41694 Reported by: Rashad Alsharpini Reviewer(s): Rashad Alsharpini, Travis Scrimshaw
sagemathgh-41694: submodule membership testing ( in operator) ### Description Fixes sagemath#41671 — submodule membership testing (`in` operator) incorrectly returns `True` for elements not in the submodule, when the base ring is a quotient of a polynomial ring. ### Changes - Added `_groebner_basis_contains()` to `Submodule_free_ambient` — performs submodule membership testing via Singular's `std` (Groebner basis) and `reduce`. For quotient rings, it lifts generators and the test vector to the covering polynomial ring and augments with ideal relation generators before computing the Groebner basis. Works for submodules over polynomial rings and quotients of polynomial rings. - Added `_element_constructor_()` override to `Submodule_free_ambient` — calls `_groebner_basis_contains()` when `check=True` and the base ring is exact, raising `TypeError` if the element is not in the submodule. ### Checklist - [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. URL: sagemath#41694 Reported by: Rashad Alsharpini Reviewer(s): Rashad Alsharpini, Travis Scrimshaw
Description
Fixes #41671 — submodule membership testing (
inoperator) incorrectly returnsTruefor elements not in the submodule, when the base ring is a quotient of a polynomial ring.Changes
_groebner_basis_contains()toSubmodule_free_ambient— performs submodule membership testing via Singular'sstd(Groebner basis) andreduce. For quotient rings, it lifts generators and the test vector to the covering polynomial ring and augments with ideal relation generators before computing the Groebner basis. Works for submodules over polynomial rings and quotients of polynomial rings._element_constructor_()override toSubmodule_free_ambient— calls_groebner_basis_contains()whencheck=Trueand the base ring is exact, raisingTypeErrorif the element is not in the submodule.Checklist