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:

Status badges

Description

sage: Ideal(3, 5)
Principal ideal (3) of Integer Ring

Misleading!

Attachments (1)

1104-Ideal_args.patch (6.7 KB) - added by AlexGhitza 14 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 15 years ago by mabshoff

Milestone: sage-2.9

comment:2 Changed 15 years ago by mabshoff

Milestone: sage-2.10sage-2.9.2

It actually does exactly what it is supposed to do in the documentation [from Ideal?]:

        Alternatively, one can also call this function with the syntax
             Ideal(gens)
        where gens is a nonempty list of generators or a single generator.

From Sage 2.9.1.1:

sage: R.<x,y> = QQ[]
sage: R
Multivariate Polynomial Ring in x, y over Rational Field
sage: R.ideal(x^2,x-y)
Ideal (x^2, x - y) of Multivariate Polynomial Ring in x, y over Rational Field
sage: Ideal([x^2,x-y])
Ideal (x^2, x - y) of Multivariate Polynomial Ring in x, y over Rational Field
sage: Ideal(x^2,x-y)
Principal ideal (x^2) of Multivariate Polynomial Ring in x, y over Rational Field

comment:3 Changed 15 years ago by AlexGhitza

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 dmharvey

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 craigcitro

Keywords: editor_malb added

comment:6 Changed 15 years ago by malb

state of affairs for editorial meeting 26/06/08 No movement on my end, sorry.

comment:7 Changed 14 years ago by mabshoff

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 AlexGhitza

Attachment: 1104-Ideal_args.patch added

comment:8 Changed 14 years ago by AlexGhitza

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 malb

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 mabshoff

Resolution: fixed
Status: newclosed

Merged in Sage 3.1.3.alpha2

Note: See TracTickets for help on using tickets.