Opened 12 years ago
Closed 11 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: | 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 12 years ago by
- Milestone set to sage-2.9
comment:2 Changed 12 years ago by
- Milestone changed from sage-2.10 to sage-2.9.2
comment:3 Changed 12 years ago by
- Summary changed from Ideal() should check arguments better to [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 12 years ago by
- Summary changed from [with patch, needs review] Ideal() should check arguments better to [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 11 years ago by
- Keywords editor_malb added
comment:6 Changed 11 years ago by
state of affairs for editorial meeting 26/06/08 No movement on my end, sorry.
comment:7 Changed 11 years ago by
- Summary changed from [with patch, negative review] Ideal() should check arguments better to [with patch, needs work] Ideal() should check arguments better
Change the subject line.
malb: any hope for this ticket?
Cheers,
Michael
Changed 11 years ago by
comment:8 Changed 11 years ago by
- Summary changed from [with patch, needs work] Ideal() should check arguments better to [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 11 years ago by
- Summary changed from [with patch, needs review] Ideal() should check arguments better to [with patch, positive review] Ideal() should check arguments better
Patch applies cleanly against 3.1.2 and reads good.
comment:10 Changed 11 years ago by
- Resolution set to fixed
- Status changed from new to 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: