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

Let number_field_elements_from_algebraics() return result using same field as input #25377

Closed
BrentBaccala opened this issue May 17, 2018 · 10 comments

Comments

@BrentBaccala
Copy link

I often want number_field_elements_from_algebraics() to return a morphism that goes back to the same field as the elements that I passed in.

For example:

sage: number_field_elements_from_algebraics(QQbar(2))
(Rational Field, 2, Ring morphism:
   From: Rational Field
   To:   Algebraic Real Field
   Defn: 1 |--> 1)

Notice that the morphism comes back to AA, not QQbar.

I've added an option to get the behavior that I want:

sage: number_field_elements_from_algebraics(QQbar(2), same_field=True)
(Rational Field, 2, Ring morphism:
   From: Rational Field
   To:   Algebraic Field
   Defn: 1 |--> 1)

Perhaps this should be the default, but it broke enough regression tests that I left it as an option.

Component: algebra

Author: Brent Baccala

Branch/Commit: ffa1e99

Reviewer: Marc Mezzarobba

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

@BrentBaccala BrentBaccala added this to the sage-8.3 milestone May 17, 2018
@BrentBaccala
Copy link
Author

Branch: public/25377

@BrentBaccala
Copy link
Author

New commits:

ffa1e99Trac #25377: add 'same_field' option to number_field_elements_from_algebraics()

@BrentBaccala
Copy link
Author

Author: Brent Baccala

@BrentBaccala
Copy link
Author

Commit: ffa1e99

@videlec
Copy link
Contributor

videlec commented May 17, 2018

comment:2

I also think that this would better be the default. Where the failures are?

@videlec
Copy link
Contributor

videlec commented May 17, 2018

comment:3

Note: this ticket is likely to create conflicts #20181

@BrentBaccala
Copy link
Author

comment:4

Replying to @videlec:

I also think that this would better be the default. Where the failures are?

This is the only troublesome one:

sage: a = QQbar((-1)^(1/4)); b = AA(a^3-a); t = b.as_number_field_element()

It raises an exception with the new code (if same_field=True is the default).

The problem is that without minimal=True, the generated number field might be complex, and thus unable to map back to AA.

So, if the elements are from AA, same_field=True might not work without minimal=True, and I wasn't inclined to change that default.

@mezzarobba
Copy link
Member

Reviewer: Marc Mezzarobba

@mezzarobba
Copy link
Member

comment:5

Replying to @BrentBaccala:

The problem is that without minimal=True, the generated number field might be complex, and thus unable to map back to AA.

So, if the elements are from AA, same_field=True might not work without minimal=True, and I wasn't inclined to change that default.

Sounds sensible. And the code looks good to me. One minor nitpick (not worth delaying the ticket IMO): I personally don't like the use of _ to store a value that you are going to access again (as opposed to cases like a, _, b = foo where you need to provide a variable but don't care about the value).

@vbraun
Copy link
Member

vbraun commented May 27, 2018

Changed branch from public/25377 to ffa1e99

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