Opened 6 months ago
Closed 6 months ago
#15745 closed defect (fixed)
The unit ideal is not prime or primary
Reported by: | cremona | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | algebra | Keywords: | unit prime primary ideal |
Cc: | pbruin | Merged in: | |
Authors: | Peter Bruin | Reviewers: | John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | u/pbruin/15745-primary_decomposition_unit_ideal (Commits) | Commit: | 295c7fd72cc777f3d5c2b2b25b2895c5c67e6170 |
Dependencies: | Stopgaps: |
Description
On sage-support on 2014-01-27, Jack <kroeker@…> reported:
I'm a bit confused about Sage's answer if Ideal(1) is prime. R.<x,y>= QQ[] I = Ideal(R(1)) I.is_prime() Sage (5.11, not only) says yes, conflicting to the definition, http://en.wikipedia.org/wiki/Prime_ideal Has somebody an expanation of this behaviour?
and in the following discussion it was agreed that this is incorrect, as is I.is_primary()}} (gives True not False) and {{{I.primary_decomposition() (gives a nonempty list) and {{{I.is_maximal())}} (raises an error instead of returning False).
This originates with Singular, but could easily be fixed in Sage.
Change History (6)
comment:1 Changed 6 months ago by pbruin
- Branch set to u/pbruin/15745-primary_decomposition_unit_ideal
- Commit set to 295c7fd72cc777f3d5c2b2b25b2895c5c67e6170
- Status changed from new to needs_review
comment:2 follow-up: ↓ 4 Changed 6 months ago by cremona
Looks good to me. I am not sure why we need to have a primer about definition and existence of primary decompositions in the docstring at all, let alone three times! But as long as they are correct (which they are, and I should know since I'll lecturing about all that later this term) I am certainly not going to suggest that they are removed.
Apart from tidying the documentation the patch just has two things, the easy one (deal with the unit ideal properly) and something else. Would it be possible to have an example of how the second change corrects something which used to be wrong?
comment:3 Changed 6 months ago by vbraun_spam
- Milestone changed from sage-6.1 to sage-6.2
comment:4 in reply to: ↑ 2 Changed 6 months ago by pbruin
Replying to cremona:
Looks good to me. I am not sure why we need to have a primer about definition and existence of primary decompositions in the docstring at all, let alone three times! But as long as they are correct (which they are, and I should know since I'll lecturing about all that later this term) I am certainly not going to suggest that they are removed.
Yes, given that the small primer was already there I thought it made more sense to tidy it up than to remove it altogether.
Apart from tidying the documentation the patch just has two things, the easy one (deal with the unit ideal properly) and something else. Would it be possible to have an example of how the second change corrects something which used to be wrong?
The thing that failed after I made the first fix was a doctest that checked that the product of the primary ideals in the primary decomposition was equal to the original ideal. The problem was that comparison of ideals was broken in the following situation: I and J are ideals of R = Q[x,y], a Gröbner basis of J has been computed for a different monomial ordering than the default one for R, and no Gröbner basis for I has been computed. The reason why it didn't work is that the equality I = J was tested using the default monomial ordering of R, but with the "wrong" Gröbner basis for J. My fix makes sure that the comparison uses the monomial ordering of the cached Gröbner basis.
How was the bug triggered by the new check for the unit ideal? At the point this check is done, no Gröbner bases have been computed for any ideal, so Sage decides to use the degrevlex ordering, while the R in the doctest has the lex ordering. So a Gröbner basis for the degrevlex ordering is cached, and on the other hand when the product of the ideals in the primary decomposition is made it has no cached Gröbner basis, which brings us in the situation above.
I could have written a new doctest, but since it is already tested by an existing doctest I thought it wasn't really necessary, and I didn't see a natural way of explaining the above problem in the patch. I should have explained in my previous comment why there is no new doctest in the patch, though.
comment:5 Changed 6 months ago by cremona
- Reviewers set to John Cremona
- Status changed from needs_review to positive_review
That's certainly a good enough explanation for me!
comment:6 Changed 6 months ago by vbraun
- Resolution set to fixed
- Status changed from positive_review to closed
In this branch: