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)

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

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by mabshoff

  • Milestone set to sage-2.9

comment:2 Changed 12 years ago by mabshoff

  • Milestone changed from sage-2.10 to sage-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 12 years ago by AlexGhitza

  • 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 dmharvey

  • 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 craigcitro

  • Keywords editor_malb added

comment:6 Changed 11 years ago by malb

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

comment:7 Changed 11 years ago by mabshoff

  • 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 AlexGhitza

comment:8 Changed 11 years ago by AlexGhitza

  • 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 malb

  • 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 mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.1.3.alpha2

Note: See TracTickets for help on using tickets.