Opened 15 years ago
Closed 14 years ago
#1104 closed defect (fixed)
[with patch, positive review] Ideal() should check arguments better
Reported by: | ncalexan | Owned by: | somebody |
---|---|---|---|
Priority: | minor | Milestone: | sage-3.1.3 |
Component: | basic arithmetic | Keywords: | ideal arguments, editor_malb |
Cc: | Merged in: | ||
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
sage: Ideal(3, 5) Principal ideal (3) of Integer Ring
Misleading!
Attachments (1)
Change History (11)
comment:1 Changed 15 years ago by
Milestone: | → sage-2.9 |
---|
comment:2 Changed 15 years ago by
Milestone: | sage-2.10 → sage-2.9.2 |
---|
comment:3 Changed 15 years ago by
Summary: | Ideal() should check arguments better → [with patch, needs review] Ideal() should check arguments better |
---|
The patch fixes the problem pointed out by Nick, and makes Ideal() complain loudly rather than return the wrong thing in other cases.
Before:
sage: Ideal(3,5) Principal ideal (3) of Integer Ring sage: Ideal(ZZ,3,5) Principal ideal (3) of Integer Ring
After:
sage: Ideal(3,5) Principal ideal (1) of Integer Ring sage: Ideal(ZZ,3,5) ... <type 'exceptions.TypeError'>: coerce must be either True or False
comment:4 Changed 15 years ago by
Summary: | [with patch, needs review] Ideal() should check arguments better → [with patch, negative review] Ideal() should check arguments better |
---|
Sorry this patch doesn't work right. With the patch I get for example:
sage: Ideal(2, 4, 6) [...] <type 'exceptions.TypeError'>: coerce must be either True or False
which is still very confusing.
The recursive function call is not the right way to do this. Should use the def func(*x, **kwds)
idiom instead; to see an example, look at
sage: R.<x> = PolynomialRing(ZZ) sage: R.ideal??
comment:5 Changed 15 years ago by
Keywords: | editor_malb added |
---|
comment:6 Changed 15 years ago by
state of affairs for editorial meeting 26/06/08 No movement on my end, sorry.
comment:7 Changed 14 years ago by
Summary: | [with patch, negative review] Ideal() should check arguments better → [with patch, needs work] Ideal() should check arguments better |
---|
Change the subject line.
malb: any hope for this ticket?
Cheers,
Michael
Changed 14 years ago by
Attachment: | 1104-Ideal_args.patch added |
---|
comment:8 Changed 14 years ago by
Summary: | [with patch, needs work] Ideal() should check arguments better → [with patch, needs review] Ideal() should check arguments better |
---|
I have completely rewritten the patch in a way that I think addresses the objections given above.
comment:9 Changed 14 years ago by
Summary: | [with patch, needs review] Ideal() should check arguments better → [with patch, positive review] Ideal() should check arguments better |
---|
Patch applies cleanly against 3.1.2 and reads good.
comment:10 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Merged in Sage 3.1.3.alpha2
It actually does exactly what it is supposed to do in the documentation [from Ideal?]:
From Sage 2.9.1.1: