Zero does not belong to zero ideal of a number field

[sagetrac-olitb]

The following is mathematically wrong:

sage: 0 in CyclotomicField(3).ideal(0)
False

It comes from the function contains(self, x) in the class NumberFieldIdeal (file sage/rings/number_field/number_field_ideal.py), which tries to compute the coordinates of the element in a basis (over ZZ) of the ideal. The function coordinates(self, x) fails for the zero ideal, raising a TypeError (in fact when _contains_ is called directly, a TypeError is raised).
A workaround is to replace

def _contains_(self, x):
    return self.coordinates(self.number_field()(x)).denominator() == 1

with

def _contains_(self, x):
    return x==0 or self.coordinates(self.number_field()(x)).denominator() == 1

but I am not sure if it is the "right" way to do it.
Is it desirable to have the _contains_ function in sage/rings/ideal.py catch the TypeError (silently)?

Apply attachment: 14366_metrod3.patch​

Component: number fields

Keywords: sd51

Author: Michiel Kosters

Reviewer: David Loeffler

Merged: sage-5.12.beta3

Issue created by migration from https://trac.sagemath.org/ticket/14366

[rharron]

comment:1

It seems to me that there is generic code in sage/rings/ideal.py for __contains__ and this code shouldn't be touched. Basically, what it does is punts to the specific case's implementation of _contains_. I think the bug you point out means that for number field ideals, _contains_ is buggy. So, the type of fix you suggest I think makes sense. Though, rather than x == 0, it should probably be x == self.number_field().zero(), otherwise anything that is 0 will evaluate as True.

(I do wonder why the code for __contains__ in sage/rings/ideal.py doesn't just return True for the zero element).

[sagetrac-mkosters]

Attachment: trac_14366.patch.gz

[sagetrac-mkosters]

comment:4

Now with a commit message.

[saraedum]

comment:6

I just bumped into this bug myself. Wouldn't it make more sense to make coordinates() return an empty tuple instead? I believe this makes sense since 0 is in the zero-dimensional vector space.

[saraedum]

Changed keywords from none to sd51

[sagetrac-mkosters]

comment:8

@saraedum, what should the output of K.ideal(0).coordinates(1) be where K is a number field? What kind of error should it produce?

[saraedum]

comment:9

Replying to @sagetrac-mkosters:

@saraedum, what should the output of K.ideal(0).coordinates(1) be where K is a number field? What kind of error should it produce?

This should raise a ValueError, I believe.

[aghitza]

comment:10

Replying to @saraedum:

I just bumped into this bug myself. Wouldn't it make more sense to make coordinates() return an empty tuple instead? I believe this makes sense since 0 is in the zero-dimensional vector space.

Yep, in vector spaces:

sage: V = QQ^1
sage: W = V.subspace([0])
sage: W.coordinates(0)
[]
sage: W.coordinates(1)
TypeError

[sagetrac-mkosters]

comment:11

The coordinate function of a relative number field returns 'absolute' coordinates to QQ. Shouldn't this actually also be in terms of the relative field (same for I.basis() where I is an ideal)?

[saraedum]

comment:12

Replying to @sagetrac-mkosters:

The coordinate function of a relative number field returns 'absolute' coordinates to QQ. Shouldn't this actually also be in terms of the relative field (same for I.basis() where I is an ideal)?

I agree. If you want to you could put this into a new ticket.

[sagetrac-mkosters]

comment:13

Attachment: 14366_method2.patch.gz

14366_method2.patch​ changes the coordinate function for the 0 ideal (but does not do relative coordinates), and fixes the containment bug.

[sagetrac-mkosters]

comment:14

Replying to @saraedum:

Replying to @sagetrac-mkosters:

The coordinate function of a relative number field returns 'absolute' coordinates to QQ. Shouldn't this actually also be in terms of the relative field (same for I.basis() where I is an ideal)?

I agree. If you want to you could put this into a new ticket.

Actually, I think it is not a good idea to do. Basically, modules (and ideals, integral closures) are not free any more (because the class number is not 1 in general), so a basis might not exist.

[loefflerd]

comment:15

I can't help thinking that an easier solution would be to use the _contains_() method of the ideal's underlying Z-module (self.free_module()).

We could just reimplement coordinates(self, x) as

def coordinates(self, x):
     K = self.number_field()
     V, from_V, to_V = K.absolute_vector_space()
     return self.free_module().coordinates(to_V(K(x)))

and then there is no longer any need to worry about the special case of the zero ideal, it will be dealt with automatically.

[sagetrac-mkosters]

Replace previous patches.

[loefflerd]

comment:16

Attachment: 14366_metrod3.patch.gz

Looks fine to me.

[loefflerd]

Reviewer: David Loeffler

[loefflerd]

Author: Michiel Kosters

[loefflerd]

comment:19

Apply 14366_metrod3.patch​

(for patchbot)

[robertwb]

comment:20

Apply 14366_metrod3.patch

[robertwb]

comment:21

Apply 14366_metrod3.patch

(for patchbot)

[jdemeyer]

Merged: sage-5.12.beta3