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

Generalize pushout of number fields with compatible embeddings #28778

Closed
kliem opened this issue Nov 20, 2019 · 34 comments
Closed

Generalize pushout of number fields with compatible embeddings #28778

kliem opened this issue Nov 20, 2019 · 34 comments

Comments

@kliem
Copy link
Contributor

kliem commented Nov 20, 2019

The pushout of two algebraic number fields with a coerce embedding in AA (the real algebraic fields) is implemented as being AA. As a consequence, it is possible to add elements from different number fields

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

Though, the pushout is not implemented for fields with coerce embedding in QQbar (algebraic field).

sage: K.<a> = NumberField(x^3 - 2, embedding=QQbar(2)**(1/3) * QQbar.zeta(3))
sage: L.<b> = NumberField(x^3 - 3, embedding=QQbar(3)**(1/3) * QQbar.zeta(3)**2)
sage: a + b
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for +

This ticket implements it.

Component: number fields

Keywords: pushout, composite field

Author: Jonathan Kliem

Branch/Commit: 8e70251

Reviewer: Vincent Delecroix, Matthias Koeppe

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

@kliem kliem added this to the sage-9.0 milestone Nov 20, 2019
@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

New commits:

aac8e0aimplement pushout for generic number fields with compatible embeddings

@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

Commit: aac8e0a

@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

Branch: public/28778

@videlec
Copy link
Contributor

videlec commented Nov 20, 2019

comment:2

A pushout is already available

sage: K.<a> = NumberField(x^2 - 2, embedding=AA(2).sqrt())
sage: L.<b> = NumberField(x^2 - 3, embedding=AA(3).sqrt())
sage: a+b
3.146264369941973?
sage: K._pushout_(L)
Algebraic Real Field

(was implemented in #20746)

Going via composite field is very fragile (order dependent, non canonical, expensive, etc).

@kliem
Copy link
Contributor Author

kliem commented Nov 20, 2019

comment:3

Thanks for pointing this out. I was aware of this method, but not that you had the discussion before.

Nevertheless, one could just devote this ticket here to implementing the case, where the fields are embedded into QQbar.

I figured this would be useful for rings in general.

@mwageringel
Copy link

comment:4

Replying to @kliem:

Nevertheless, one could just devote this ticket here to implementing the case, where the fields are embedded into QQbar.

I figured this would be useful for rings in general.

Assuming this also makes coercion between AA and number fields embedded in QQbar possible, this would then help solve #28489 as far as I can see (possibly after #28774).

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Nov 20, 2019

comment:5

description updated then

@videlec videlec changed the title Implement pushout for generic number fields with compatible embeddings Generalize pushout of number fields with compatible embeddings Nov 20, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d3258e6pushout for number fields with compatible embeddings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 21, 2019

Changed commit from aac8e0a to d3258e6

@kliem
Copy link
Contributor Author

kliem commented Nov 21, 2019

comment:7

I also fixed a bug of the previous construction. It did try to coerce number fields embedded into RLF into AA, which did not work (the attribute _embedded_real is set to True in that case).

There is a failing doctest now in sage/rings/qqbar.py line 167, for which I need some help.

The test wants to make sure, that QQ[I] and QQbar cannot be implicitly coerced, as QQ[I] does not come with an embedding.

This makes sense, however this is not the way it is implemented:

sage: QQ[I]
Number Field in I with defining polynomial x^2 + 1 with I = 1*I

So QQ[I] does come with an embedding to QQbar and therefore I think my construction is correct in using this embedding.
Unlike this example

sage: K.<a> = NumberField(x^2 + 1); K
Number Field in a with defining polynomial x^2 + 1

where my construction does not give a pushout.

@kliem
Copy link
Contributor Author

kliem commented Nov 21, 2019

comment:8

To be more precise, as it is QQ[I] comes with an embedding to complex lazy field:

sage: coercion_model.explain(QQ[I], CLF)
Coercion on left operand via
    Generic morphism:
      From: Number Field in I with defining polynomial x^2 + 1 with I = 1*I
      To:   Complex Lazy Field
      Defn: I -> 1*I
Arithmetic performed after coercions.
Result lives in Complex Lazy Field
Complex Lazy Field

and there is a coercion from QQbar to CLF

sage: coercion_model.explain(QQbar, CLF)
Coercion on left operand via
    Generic morphism:
      From: Algebraic Field
      To:   Complex Lazy Field
Arithmetic performed after coercions.
Result lives in Complex Lazy Field
Complex Lazy Field

I find it very confusing, that there is a doctest fixing that there shall be no coercion of QQ[I] and CLF. If this is the way we want it to be, there should not be coercion of QQ[I] and CLF in the first place.

The same holds for QQ[sqrt(2)] btw. There is no embedding of QQ[sqrt(2), sqrt(3)] to RLF, but probably not by design but only because relative number fields do not support embeddings.

@kliem
Copy link
Contributor Author

kliem commented Nov 21, 2019

comment:9

Please tell me, if I should post a query on sage-devel instead.

@kliem
Copy link
Contributor Author

kliem commented Nov 22, 2019

Changed branch from public/28778 to public/28778-bis

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

d46d6bbremove unneeded test

@kliem
Copy link
Contributor Author

kliem commented Nov 23, 2019

comment:14

Ok. I see.

This example is handled by the _pushout_ method now, but I don't think it hurts. I wasn't aware that a canonical coercion is already discovered. Nice.

I could also just handle AA and QQbar, if this seems more reasonable.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2019

Changed commit from d46d6bb to 9a1bce3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

9a1bce3small mistake in docstests

@videlec
Copy link
Contributor

videlec commented Nov 25, 2019

comment:16

Replying to @kliem:

Ok. I see.

This example is handled by the _pushout_ method now, but I don't think it hurts. I wasn't aware that a canonical coercion is already discovered. Nice.

I could also just handle AA and QQbar, if this seems more reasonable.

Yes, it would be better.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2019

Changed commit from 9a1bce3 to 8e70251

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

8e70251defining the pushout for number fields just for AA and QQbar

@kliem

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Jan 6, 2020

comment:19

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-9.0, sage-9.1 Jan 6, 2020
@mkoeppe
Copy link
Member

mkoeppe commented Apr 14, 2020

comment:20

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@videlec
Copy link
Contributor

videlec commented Apr 23, 2020

comment:21

Maybe we could be more greedy with something like

if codomain_self is not self or codomain_other is not other:
    try:
        return coercion_model.common_parent(codomain_self, codomain_other)
    except TypeError:
        pass

@mkoeppe
Copy link
Member

mkoeppe commented Jul 18, 2020

comment:22

Looks like a good improvement as is.

Feel free to open the suggestion from comment 21 as another ticket.

@mkoeppe
Copy link
Member

mkoeppe commented Jul 18, 2020

Reviewer: Vincent Delecroix, Matthias Koeppe

@kliem
Copy link
Contributor Author

kliem commented Jul 18, 2020

comment:23

Thank you.

@vbraun
Copy link
Member

vbraun commented Jul 25, 2020

Changed branch from public/28778-bis to 8e70251

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

6 participants