Opened 15 years ago

Closed 14 years ago

[with patch, positive review] Ideal() should check arguments better

Reported by: Owned by: ncalexan somebody minor sage-3.1.3 basic arithmetic ideal arguments, editor_malb N/A

Description

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

comment:1 Changed 15 years ago by mabshoff

Milestone: → sage-2.9

comment:2 Changed 15 years ago by mabshoff

Milestone: sage-2.10 → 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 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: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

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 new → closed

Merged in Sage 3.1.3.alpha2

Note: See TracTickets for help on using tickets.