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

Incorrect conversion from ℚ[-i] to SR #31551

Closed
mezzarobba opened this issue Mar 24, 2021 · 14 comments
Closed

Incorrect conversion from ℚ[-i] to SR #31551

mezzarobba opened this issue Mar 24, 2021 · 14 comments

Comments

@mezzarobba
Copy link
Member

This should return -I, not I:

sage: K.<j> = QuadraticField(-1, embedding=CC(0,-1))
sage: SR(j)
I

Works under Sage 9.2, so maybe related to #18036?

CC: @videlec

Component: symbolics

Author: Marc Mezzarobba

Branch/Commit: fddaa2c

Reviewer: Vincent Delecroix

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

@mezzarobba mezzarobba added this to the sage-9.3 milestone Mar 24, 2021
@mezzarobba

This comment has been minimized.

@slel
Copy link
Member

slel commented Mar 24, 2021

comment:2

Possibly related:

@videlec
Copy link
Contributor

videlec commented Mar 25, 2021

comment:3

Indeed, the following method on gaussian integers looks bad

    def _symbolic_(self, SR):
        r"""
        EXAMPLES::

            sage: SR(1 + 2*i)
            2*I + 1
        """
        from sage.symbolic.constants import I
        return self[1]*I + self[0]

@mezzarobba
Copy link
Member Author

comment:4

Hmm, if I remember right, the GaussianInteger class was intended for ℚ[i] with its standard embedding, so I think there is something else.

@mezzarobba
Copy link
Member Author

comment:5

...but I see no reason not to extend it to support both embeddings.


New commits:

fddaa2c#31151 support both embeddings in NumberFieldElement_gaussian

@mezzarobba
Copy link
Member Author

Branch: u/mmezzarobba/ticket/31151

@mezzarobba
Copy link
Member Author

Commit: fddaa2c

@mezzarobba
Copy link
Member Author

Author: Marc Mezzarobba

@videlec
Copy link
Contributor

videlec commented Mar 25, 2021

comment:6

looks good... waiting for patchbot.

@videlec
Copy link
Contributor

videlec commented Mar 25, 2021

Reviewer: Vincent Delecroix

@mezzarobba
Copy link
Member Author

comment:8

thanks for the quick review!

@mezzarobba
Copy link
Member Author

comment:9

Raising the priority to critical to stress that the fix really should go in Sage 9.3.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 29, 2021

comment:10

Setting priority to blocker to bring this ticket to the attention of the release bot.

@vbraun
Copy link
Member

vbraun commented Apr 1, 2021

Changed branch from u/mmezzarobba/ticket/31151 to fddaa2c

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

5 participants