Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UniversalCyclotomicField is not finite #25686

Closed
soehms opened this issue Jun 28, 2018 · 19 comments
Closed

UniversalCyclotomicField is not finite #25686

soehms opened this issue Jun 28, 2018 · 19 comments

Comments

@soehms
Copy link
Member

soehms commented Jun 28, 2018

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  Jean-Philippe Labbé <labbe@math.fu-berlin.de>   2015-04-29 17:56:59 +0300
committer       Jean-Philippe Labbé <labbe@math.fu-berlin.de>   2015-04-29 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!

CC: @tscrim

Component: algebra

Keywords: days94

Author: Sebastian Oehms

Branch/Commit: 03055ab

Reviewer: Tomer Bauer, Luis Felipe Tabera

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

@soehms soehms added this to the sage-8.3 milestone Jun 28, 2018
@soehms
Copy link
Member Author

soehms commented Jun 28, 2018

Branch: u/soehms/ucf_not_finite

@soehms
Copy link
Member Author

soehms commented Jun 28, 2018

comment:3

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 to is_finite in rings.pyx replacing the raise of NotImplementedError to a super(Ring, self).is_finite()-call.


New commits:

a5673d1remove meth is_finite from ring.pyx and universal_cyclotomic_field in order to fall back to the categorial framework

@soehms
Copy link
Member Author

soehms commented Jun 28, 2018

Commit: a5673d1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 28, 2018

Changed commit from a5673d1 to 16b48b5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 28, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

16b48b5correction

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 28, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

ae0cbb8correction of comment

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 28, 2018

Changed commit from 16b48b5 to ae0cbb8

@soehms
Copy link
Member Author

soehms commented Jun 28, 2018

New commits:

ae0cbb8correction of comment

@lftabera
Copy link
Contributor

comment:7

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2018

Changed commit from ae0cbb8 to d3b2c00

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

d3b2c00test added

@mathzeta
Copy link

Reviewer: Tomer Bauer

@lftabera
Copy link
Contributor

Changed reviewer from Tomer Bauer to Tomer Bauer, Luis Felipe Tabera

@vbraun
Copy link
Member

vbraun commented Jun 30, 2018

comment:11

See patchbot

@mathzeta
Copy link

comment:12

The line return super(Ring, self).is_finite() is probably the culprit, we need to update the error message of that test.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

03055abdoctest error in _list_from_iterator fixed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 1, 2018

Changed commit from d3b2c00 to 03055ab

@mathzeta
Copy link

mathzeta commented Jul 2, 2018

comment:15

Works for me. If the patchbot is happy, then positive review.

@vbraun
Copy link
Member

vbraun commented Jul 8, 2018

Changed branch from u/soehms/ucf_not_finite to 03055ab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants