Opened 3 years ago

Closed 2 years ago

#20746 closed enhancement (fixed)

Pushout for real embedded number fields

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.3
Component: number fields Keywords: days74, days84
Cc: jdemeyer, tscrim Merged in:
Authors: Vincent Delecroix Reviewers: Jean-Philippe Labbé
Report Upstream: N/A Work issues:
Branch: 4a52745 (Commits) Commit: 4a52745b734543ca8e65f2ac3050c9a73f223edd
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

Implement the following pushout

sage: K.<a> = NumberField(x^2 - 3, embedding=AA(3)**(1/2))
sage: L.<b> = NumberField(x^2 - 2, embedding=AA(2)**(1/2))
sage: a+b
3.146264369941973?
sage: parent(_)
Algebraic Real Field
sage: a < b
False

Change History (12)

comment:1 Changed 3 years ago by vdelecroix

  • Branch set to u/vdelecroix/20746
  • Commit set to 4a52745b734543ca8e65f2ac3050c9a73f223edd
  • Status changed from new to needs_review

New commits:

4a52745Trac 20746: pushout for real embedded number fields

comment:2 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 3 years ago by vdelecroix

  • Cc tscrim added

comment:4 Changed 3 years ago by pbruin

  • Status changed from needs_review to needs_work

Isn't it a bit drastic to declare AA to be the pushout rather than to create a suitable number field? I think a better solution would be to use the existing composite_fields() method, which respects embeddings:

  • src/sage/rings/number_field/number_field_base.pyx

    a b cdef class NumberField(Field): 
    350350        else:
    351351            raise ValueError("No embedding set. You need to specify a a real embedding.")
    352352
     353    def _pushout_(self, other):
     354        if (isinstance(other, NumberField) and self._embedded_real and
     355            (<NumberField>other)._embedded_real):
     356            return self.composite_fields(other)[0]

This makes the examples in the current branch work just as well (you can see the numerical values after coercing into AA).

comment:5 Changed 3 years ago by vdelecroix

Fair enough. Though it is more expensive.

comment:6 Changed 3 years ago by vdelecroix

And this is order dependent

sage: K.<a> = NumberField(x^2 - 3, embedding=AA(3)**(1/2))
sage: L.<b> = NumberField(x^2 - 2, embedding=AA(2)**(1/2))
sage: a+b
ab^3 - 10*ab

versus

sage: K.<a> = NumberField(x^2 - 3, embedding=AA(3)**(1/2))
sage: L.<b> = NumberField(x^2 - 2, embedding=AA(2)**(1/2))
sage: b+a
-ba^3 + 10*ba

comment:7 Changed 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:8 Changed 2 years ago by jipilab

This coercion has its advantages:

  • makes it possible to do arithmetic operations with elements with two different NumberFields as parents. Right now returns a TypeError?:
    sage: J = NumberField(x^2 - 2,'s')
    sage: s = J.gens()[0]
    sage: K = NumberField(x^3 - 2,'t')
    sage: t = K.gens()[0]
    sage: s * t
    Traceback (most recent call last):
    ...
    TypeError: unsupported operand parent(s) for *: ...
  • It is lazy, it does have to compute the join.

Depending on the usage, one may want to get the join of the fields using composite_field, this introduces another variable and is order dependant. I guess that if one wants to do computations on the long run with that field, one would already use the right field from start.

IMHO, this ticket already fixes an operation that one would naturally want to have at hand. So, I would make this a positive review.

comment:9 Changed 2 years ago by jipilab

  • Status changed from needs_review to positive_review

comment:10 Changed 2 years ago by jipilab

  • Reviewers set to Jean-Philippe Labbé

comment:11 Changed 2 years ago by jipilab

  • Keywords days84 added

comment:12 Changed 2 years ago by vbraun

  • Branch changed from u/vdelecroix/20746 to 4a52745b734543ca8e65f2ac3050c9a73f223edd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.