Opened 9 years ago
Closed 8 years ago
#11139 closed defect (fixed)
Ideal with no generators for univariate polynomial rings
Reported by: | mraum | Owned by: | malb |
---|---|---|---|
Priority: | critical | Milestone: | sage-4.7.1 |
Component: | commutative algebra | Keywords: | |
Cc: | Merged in: | sage-4.7.1.alpha1 | |
Authors: | Martin Raum, Jeroen Demeyer | Reviewers: | John Palmieri, Martin Raum |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Consider the following (Sage 4.7alpha1) :
sage: P.<a> = QQ[] sage: P.ideal([]) Booom!
The issue is twofold: The ideal method in rings/rings.py is called and does not accept an empty list. But also the univariate polynomial rings should call a specialized method just like the multivariate ones do.
It worked in earlier versions (like 4.2 or so) and is a construction that easily comes up as a corner case.
Apply 11139_alternative.patch.
Attachments (3)
Change History (13)
Changed 9 years ago by
comment:1 Changed 8 years ago by
- Reviewers set to John Palmieri
- Status changed from new to needs_review
Changed 8 years ago by
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 Changed 8 years ago by
- Description modified (diff)
What do you think of this alternative patch? IMHO, it has better logic.
comment:4 follow-up: ↓ 5 Changed 8 years ago by
- Status changed from needs_review to needs_info
I think that indeed the logic is much better. Though it might be confusing to beginners. But this improvement brings my attention to a problem, that has been there before:
R.<x> = QQ[] R.ideal((x for _ in range(2)))
This is an instance of GeneratorType?, that is called with len. So either we exclude the GeneratorType? (no change to the situtation before) or correct it. Also, mind that a ring element might be an instance of GeneratorType?. In that (very exceptional) situation, the new logic would cause trouble.
Any experience with similar code at other places in Sage?
comment:5 in reply to: ↑ 4 Changed 8 years ago by
- Status changed from needs_info to needs_review
Replying to mraum:
This is an instance of GeneratorType?, that is called with len. So either we exclude the GeneratorType? (no change to the situtation before) or correct it.
I would say not to check for GeneratorType
. I have added a new patch for this. I still have to test it thoroughly.
comment:7 Changed 8 years ago by
- Milestone changed from sage-4.7 to sage-4.7.1
- Priority changed from blocker to critical
comment:9 Changed 8 years ago by
- Reviewers changed from John Palmieri to John Palmieri, Martin Raum
- Status changed from needs_review to positive_review
I think, since I haven't written this ultimate patch, I am a legitimate reviewer. All tests pass and also the patch bot does not complain. Although we have changed the logic, backward compatibility is preserved.
comment:10 Changed 8 years ago by
- Merged in set to sage-4.7.1.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
I'm happy with this. Here's a new patch with a few doctests illustrating that the problem has been fixed.
I'm going to mark this as "needs review"; it wouldn't hurt to have someone else look it over.