# Ticket #1104(closed defect: fixed)

Opened 6 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

### Description

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

## Change History

### comment:1 Changed 6 years ago by mabshoff

• Milestone set to sage-2.9

### comment:2 Changed 5 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 5 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 5 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 5 years ago by craigcitro

• Keywords arguments, editor_malb added; arguments removed

### comment:6 Changed 5 years ago by malb

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

### comment:7 Changed 5 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

### comment:8 Changed 5 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 5 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 5 years ago by mabshoff

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

Merged in Sage 3.1.3.alpha2

Note: See TracTickets for help on using tickets.