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

Pushout for real embedded number fields #20746

Closed
videlec opened this issue Jun 1, 2016 · 11 comments
Closed

Pushout for real embedded number fields #20746

videlec opened this issue Jun 1, 2016 · 11 comments

Comments

@videlec
Copy link
Contributor

videlec commented Jun 1, 2016

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

CC: @jdemeyer @tscrim

Component: number fields

Keywords: days74, days84

Author: Vincent Delecroix

Branch/Commit: 4a52745

Reviewer: Jean-Philippe Labbé

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

@videlec videlec added this to the sage-7.3 milestone Jun 1, 2016
@videlec
Copy link
Contributor Author

videlec commented Jun 1, 2016

Branch: u/vdelecroix/20746

@videlec
Copy link
Contributor Author

videlec commented Jun 1, 2016

Commit: 4a52745

@videlec
Copy link
Contributor Author

videlec commented Jun 1, 2016

New commits:

4a52745Trac 20746: pushout for real embedded number fields

@videlec

This comment has been minimized.

@pjbruin
Copy link
Contributor

pjbruin commented Jun 1, 2016

comment:4

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:

--- a/src/sage/rings/number_field/number_field_base.pyx
+++ b/src/sage/rings/number_field/number_field_base.pyx
@@ -350,3 +350,7 @@ cdef class NumberField(Field):
         else:
             raise ValueError("No embedding set. You need to specify a a real embedding.")
 
+    def _pushout_(self, other):
+        if (isinstance(other, NumberField) and self._embedded_real and
+            (<NumberField>other)._embedded_real):
+            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).

@videlec
Copy link
Contributor Author

videlec commented Jun 1, 2016

comment:5

Fair enough. Though it is more expensive.

@videlec
Copy link
Contributor Author

videlec commented Jun 1, 2016

comment:6

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

@jplab
Copy link
Contributor

jplab commented Mar 5, 2017

comment:8

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.

@jplab
Copy link
Contributor

jplab commented Mar 5, 2017

Reviewer: Jean-Philippe Labbé

@jplab
Copy link
Contributor

jplab commented Mar 5, 2017

Changed keywords from days74 to days74, days84

@vbraun
Copy link
Member

vbraun commented Mar 8, 2017

Changed branch from u/vdelecroix/20746 to 4a52745

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