#25377 closed enhancement (fixed)

Let number_field_elements_from_algebraics() return result using same field as input

Reported by: gh-BrentBaccala Owned by:
Priority: minor Milestone: sage-8.3
Component: algebra Keywords:
Cc: Merged in:
Authors: Brent Baccala Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: ffa1e99 (Commits) Commit: ffa1e99aa944fb14c7a297a467b5d83f97bcc25b
Dependencies: Stopgaps:

Description

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.

Change History (6)

comment:1 Changed 19 months ago by gh-BrentBaccala

  • Authors set to Brent Baccala
  • Branch set to public/25377
  • Commit set to ffa1e99aa944fb14c7a297a467b5d83f97bcc25b
  • Status changed from new to needs_review

New commits:

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

comment:2 follow-up: Changed 19 months ago by vdelecroix

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

comment:3 Changed 19 months ago by vdelecroix

Note: this ticket is likely to create conflicts #20181

comment:4 in reply to: ↑ 2 ; follow-up: Changed 19 months ago by gh-BrentBaccala

Replying to vdelecroix:

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.

Last edited 19 months ago by gh-BrentBaccala (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 19 months ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

Replying to gh-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).

comment:6 Changed 19 months ago by vbraun

  • Branch changed from public/25377 to ffa1e99aa944fb14c7a297a467b5d83f97bcc25b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.