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: |
Description (last modified by )
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
- Branch set to public/28778
- Commit set to aac8e0a623d2de23db244d95dd59bf725a4a0cd2
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
- 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: ↓ 4 Changed 2 years ago by
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
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
- 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
- Commit changed from aac8e0a623d2de23db244d95dd59bf725a4a0cd2 to d3258e66d6b976b401e66c9894c4194a0399e661
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d3258e6 | pushout for number fields with compatible embeddings
|
comment:7 Changed 2 years ago by
- 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
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
- 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
- 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:
b4b2cf1 | pushout for number fields with compatible embeddings
|
comment:11 Changed 2 years ago by
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.
comment:12 Changed 2 years ago by
- Status changed from needs_review to needs_work
comment:13 Changed 2 years ago by
- Commit changed from b4b2cf10f5e5ad6b88825ad7ab7ba82a3400a854 to d46d6bbbe80fdbf6bd367e2f0793b9482f644bd4
Branch pushed to git repo; I updated commit sha1. New commits:
d46d6bb | remove unneeded test
|
comment:14 follow-up: ↓ 16 Changed 2 years ago by
- 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
- Commit changed from d46d6bbbe80fdbf6bd367e2f0793b9482f644bd4 to 9a1bce35034c4292eee9ecb8a56a19ce8bc75f94
Branch pushed to git repo; I updated commit sha1. New commits:
9a1bce3 | small mistake in docstests
|
comment:16 in reply to: ↑ 14 Changed 2 years ago by
- 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
andQQbar
, if this seems more reasonable.
Yes, it would be better.
comment:17 Changed 2 years ago by
- Commit changed from 9a1bce35034c4292eee9ecb8a56a19ce8bc75f94 to 8e7025115e4d2d203d67779d559ad3072c06073d
Branch pushed to git repo; I updated commit sha1. New commits:
8e70251 | defining the pushout for number fields just for AA and QQbar
|
comment:18 Changed 2 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:19 Changed 2 years ago by
- Milestone changed from sage-9.0 to sage-9.1
Ticket retargeted after milestone closed
comment:20 Changed 2 years ago by
- 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
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
- 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
Thank you.
comment:24 Changed 22 months ago by
- Branch changed from public/28778-bis to 8e7025115e4d2d203d67779d559ad3072c06073d
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
implement pushout for generic number fields with compatible embeddings