Opened 3 years ago
Closed 3 years ago
#25686 closed defect (fixed)
UniversalCyclotomicField is not finite
Reported by:  soehms  Owned by:  

Priority:  major  Milestone:  sage8.3 
Component:  algebra  Keywords:  days94 
Cc:  tscrim  Merged in:  
Authors:  Sebastian Oehms  Reviewers:  Tomer Bauer, Luis Felipe Tabera 
Report Upstream:  N/A  Work issues:  
Branch:  03055ab (Commits, GitHub, GitLab)  Commit:  03055ab738fc14f50e86a18ff42e8f5a5adc382c 
Dependencies:  Stopgaps: 
Description
sage: UCF = UniversalCyclotomicField() sage: UCF.is_finite() True
This is explicitly given in the code of universal_cyclotomic_field.py:
def is_finite(self): r""" Returns ``True``. EXAMPLES:: sage: UniversalCyclotomicField().is_finite() True .. TODO:: this method should be provided by the category. """ return True
This has been correct before a change in April 2015 (Trac #1852). It looks like a mistake in connection with that ticket. The old code from git looked like this:
author JeanPhilippe Labbé <labbe@math.fuberlin.de> 20150429 17:56:59 +0300 committer JeanPhilippe Labbé <labbe@math.fuberlin.de> 20150429 17:56:59 +0300 commit dc3de84809792bde5e2cde8c8c579f42496e0647 (patch) tree 2a5ed5f0daf452364580856388e5717cf47f1b60 parent Updated Sage version to 6.7.beta3 (diff) parent Trac 18152: __neg__ for UCF elements (diff)  def is_finite(self):  r"""  Returns False as ``self`` is not finite.   EXAMPLES::   sage: UCF = UniversalCyclotomicField()  sage: UCF.is_finite()  False  """  return False
Thus the task might be to restore the old code!
Change History (17)
comment:1 Changed 3 years ago by
 Cc tscrim added
comment:2 Changed 3 years ago by
 Branch set to u/soehms/ucf_not_finite
comment:3 Changed 3 years ago by
 Commit set to a5673d143b2368b3a21a9e7e6fa21106170778ff
comment:4 Changed 3 years ago by
 Commit changed from a5673d143b2368b3a21a9e7e6fa21106170778ff to 16b48b595e5a2159ba60d7015996c9d8075ab828
Branch pushed to git repo; I updated commit sha1. New commits:
16b48b5  correction

comment:5 Changed 3 years ago by
 Commit changed from 16b48b595e5a2159ba60d7015996c9d8075ab828 to ae0cbb88c251edf2a7380f687fd5aae7d2a79760
Branch pushed to git repo; I updated commit sha1. New commits:
ae0cbb8  correction of comment

comment:6 Changed 3 years ago by
 Status changed from new to needs_review
New commits:
ae0cbb8  correction of comment

comment:7 Changed 3 years ago by
You should add a test that shows that the bug is fixed. Since you have deleted the method, this test has to be added elsewhere. In this case, it can be added to the general set of tests at the beggining of the universal_cyclotomic_field.py file
comment:8 Changed 3 years ago by
 Commit changed from ae0cbb88c251edf2a7380f687fd5aae7d2a79760 to d3b2c00f2ef0f22389d3b30890d322479f6aa902
Branch pushed to git repo; I updated commit sha1. New commits:
d3b2c00  test added

comment:9 Changed 3 years ago by
 Reviewers set to Tomer Bauer
 Status changed from needs_review to positive_review
comment:10 Changed 3 years ago by
 Reviewers changed from Tomer Bauer to Tomer Bauer, Luis Felipe Tabera
comment:12 Changed 3 years ago by
The line return super(Ring, self).is_finite()
is probably the culprit, we need to update the error message of that test.
comment:13 Changed 3 years ago by
 Commit changed from d3b2c00f2ef0f22389d3b30890d322479f6aa902 to 03055ab738fc14f50e86a18ff42e8f5a5adc382c
Branch pushed to git repo; I updated commit sha1. New commits:
03055ab  doctest error in _list_from_iterator fixed

comment:14 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:15 Changed 3 years ago by
Works for me. If the patchbot is happy, then positive review.
comment:16 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:17 Changed 3 years ago by
 Branch changed from u/soehms/ucf_not_finite to 03055ab738fc14f50e86a18ff42e8f5a5adc382c
 Resolution set to fixed
 Status changed from positive_review to closed
We (Travis and me) tried to move the
is_finite
method to the category framework. But this lead into difficulties. Thus, we let it fall back tois_finite
in rings.pyx replacing the raise ofNotImplementedError
to asuper(Ring, self).is_finite()
call.New commits:
remove meth is_finite from ring.pyx and universal_cyclotomic_field in order to fall back to the categorial framework