Opened 7 years ago

Closed 6 years ago

#14366 closed defect (fixed)

Zero does not belong to zero ideal of a number field

Reported by: olitb Owned by: davidloeffler
Priority: major Milestone: sage-5.12
Component: number fields Keywords: sd51
Cc: Merged in: sage-5.12.beta3
Authors: Michiel Kosters Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by davidloeffler)

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 14366_metrod3.patch

Attachments (3)

trac_14366.patch (1.2 KB) - added by mkosters 6 years ago.
14366_method2.patch (1.8 KB) - added by mkosters 6 years ago.
14366_metrod3.patch (4.9 KB) - added by mkosters 6 years ago.
Replace previous patches.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by robharron

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).

comment:2 Changed 6 years ago by mkosters

  • Status changed from new to needs_review

comment:3 Changed 6 years ago by mkosters

  • Status changed from needs_review to needs_work

Changed 6 years ago by mkosters

comment:4 Changed 6 years ago by mkosters

Now with a commit message.

comment:5 Changed 6 years ago by mkosters

  • Status changed from needs_work to needs_review

comment:6 follow-up: Changed 6 years ago by 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.

comment:7 Changed 6 years ago by saraedum

  • Keywords sd51 added
  • Status changed from needs_review to needs_info

comment:8 follow-up: Changed 6 years ago by 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?

comment:9 in reply to: ↑ 8 Changed 6 years ago by saraedum

Replying to 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.

comment:10 in reply to: ↑ 6 Changed 6 years ago by AlexGhitza

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

comment:11 follow-up: Changed 6 years ago by 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)?

comment:12 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by saraedum

Replying to 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.

Changed 6 years ago by mkosters

comment:13 Changed 6 years ago by mkosters

  • Status changed from needs_info to needs_review

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

comment:14 in reply to: ↑ 12 Changed 6 years ago by mkosters

Replying to saraedum:

Replying to 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.

comment:15 Changed 6 years ago by davidloeffler

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.

Changed 6 years ago by mkosters

Replace previous patches.

comment:16 Changed 6 years ago by davidloeffler

  • Authors set to Michiel Kosters
  • Reviewers set to David Loeffler
  • Status changed from needs_review to positive_review

Looks fine to me.

comment:17 Changed 6 years ago by davidloeffler

  • Description modified (diff)

comment:18 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:19 Changed 6 years ago by davidloeffler

Apply 14366_metrod3.patch​

(for patchbot)

comment:20 Changed 6 years ago by robertwb

Apply 14366_metrod3.patch

comment:21 Changed 6 years ago by robertwb

Apply 14366_metrod3.patch

(for patchbot)

comment:22 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.