Opened 6 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 )
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)
Change History (25)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Status changed from needs_review to needs_work
Changed 6 years ago by
comment:4 Changed 6 years ago by
Now with a commit message.
comment:5 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:6 follow-up: ↓ 10 Changed 6 years ago by
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
- Keywords sd51 added
- Status changed from needs_review to needs_info
comment:8 follow-up: ↓ 9 Changed 6 years ago by
@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
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
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: ↓ 12 Changed 6 years ago by
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: ↓ 14 Changed 6 years ago by
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
comment:13 Changed 6 years ago by
- 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
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
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.
comment:16 Changed 6 years ago by
- Reviewers set to David Loeffler
- Status changed from needs_review to positive_review
Looks fine to me.
comment:17 Changed 6 years ago by
- Description modified (diff)
comment:18 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:19 Changed 6 years ago by
Apply 14366_metrod3.patch
(for patchbot)
comment:20 Changed 6 years ago by
Apply 14366_metrod3.patch
comment:21 Changed 6 years ago by
Apply 14366_metrod3.patch
(for patchbot)
comment:22 Changed 6 years ago by
- Merged in set to sage-5.12.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
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 thanx == 0
, it should probably bex == 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).