Opened 2 years ago

Closed 22 months ago

#28778 closed enhancement (fixed)

Generalize pushout of number fields with compatible embeddings

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.2
Component: number fields Keywords: number fields, coercion, pushout, composite field
Cc: Merged in:
Authors: Jonathan Kliem Reviewers: Vincent Delecroix, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 8e70251 (Commits, GitHub, GitLab) Commit: 8e7025115e4d2d203d67779d559ad3072c06073d
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

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.

Change History (24)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/28778
  • Commit set to aac8e0a623d2de23db244d95dd59bf725a4a0cd2
  • Status changed from new to needs_review

New commits:

aac8e0aimplement pushout for generic number fields with compatible embeddings

comment:2 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_info

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).

comment:3 follow-up: Changed 2 years ago by gh-kliem

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.

comment:4 in reply to: ↑ 3 Changed 2 years ago by gh-mwageringel

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

comment:5 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_info to needs_work
  • Summary changed from Implement pushout for generic number fields with compatible embeddings to Generalize pushout of number fields with compatible embeddings

description updated then

comment:6 Changed 2 years ago by git

  • Commit changed from aac8e0a623d2de23db244d95dd59bf725a4a0cd2 to d3258e66d6b976b401e66c9894c4194a0399e661

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

d3258e6pushout for number fields with compatible embeddings

comment:7 Changed 2 years ago by gh-kliem

  • Status changed from needs_work to needs_review

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.

comment:8 Changed 2 years ago by gh-kliem

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.

comment:9 Changed 2 years ago by gh-kliem

  • Status changed from needs_review to needs_info

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

comment:10 Changed 2 years ago by gh-kliem

  • Branch changed from public/28778 to public/28778-bis
  • Commit changed from d3258e66d6b976b401e66c9894c4194a0399e661 to b4b2cf10f5e5ad6b88825ad7ab7ba82a3400a854
  • Description modified (diff)
  • Status changed from needs_info to needs_review

Ok, I just went back to just doing QQbar and added the case that both are embedded into the same field.


New commits:

b4b2cf1pushout for number fields with compatible embeddings

comment:11 Changed 2 years ago by vdelecroix

Your example

sage: K.<sqrt2_plus_sqrt3> = NumberField(x^4 - 10*x^2 + 1)
sage: K2.<sqrt2> = NumberField(x^2-2, embedding=(sqrt2_plus_sqrt3^3 - 9*sqrt2_plus_sqrt3)/2)
sage: K3.<sqrt3> = NumberField(x^2-3, embedding=-(sqrt2_plus_sqrt3^3 - 11*sqrt2_plus_sqrt3)/2)
sage: sqrt2 + sqrt3
sqrt2_plus_sqrt3

is working without any modification to the _pushout_ method. You should simply remove what you wrote for number fields unless there is a real application for it.

Last edited 2 years ago by vdelecroix (previous) (diff)

comment:12 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

comment:13 Changed 2 years ago by git

  • Commit changed from b4b2cf10f5e5ad6b88825ad7ab7ba82a3400a854 to d46d6bbbe80fdbf6bd367e2f0793b9482f644bd4

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

d46d6bbremove unneeded test

comment:14 follow-up: Changed 2 years ago by gh-kliem

  • Status changed from needs_work to needs_review

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.

comment:15 Changed 2 years ago by git

  • Commit changed from d46d6bbbe80fdbf6bd367e2f0793b9482f644bd4 to 9a1bce35034c4292eee9ecb8a56a19ce8bc75f94

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

9a1bce3small mistake in docstests

comment:16 in reply to: ↑ 14 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

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

comment:17 Changed 2 years ago by git

  • Commit changed from 9a1bce35034c4292eee9ecb8a56a19ce8bc75f94 to 8e7025115e4d2d203d67779d559ad3072c06073d

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

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

comment:18 Changed 2 years ago by gh-kliem

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:19 Changed 2 years ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:20 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

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.

comment:21 Changed 2 years ago by vdelecroix

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

comment:22 Changed 22 months ago by mkoeppe

  • Reviewers set to Vincent Delecroix, Matthias Koeppe
  • Status changed from needs_review to positive_review

Looks like a good improvement as is.

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

comment:23 Changed 22 months ago by gh-kliem

Thank you.

comment:24 Changed 22 months ago by vbraun

  • Branch changed from public/28778-bis to 8e7025115e4d2d203d67779d559ad3072c06073d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.