Adapt QQBar documentation #1715
Conversation
|
Test failure looks real |
Some copy-and-paste error when adapting the tests probably. But needs to wait for me to get back to a computer |
lgoettgens
force-pushed
the
lg/QQBar-docs
branch
2 times, most recently
from
146c6cc
to
3d360df
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1715 +/- ##
=======================================
Coverage 85.95% 85.95%
=======================================
Files 95 95
Lines 36503 36503
=======================================
Hits 31375 31375
Misses 5128 5128 ☔ View full report in Codecov by Sentry. |
lgoettgens
requested review from
thofma and
fingolfin
and removed request for
thofma
| @@ -132,7 +120,7 @@ julia> CC = AcbField(32); CC(QQBar(-1) ^ (QQBar(1) // 4)) | |||
| Retrieving the minimal polynomial and algebraic conjugates | |||
| of a given algebraic number: | |||
|
|
|||
| ```jldoctest | |||
| ```jldoctest; setup = :(QQBar = algebraic_closure(QQ)) | |||
There was a problem hiding this comment.
If we unexport QQBar (and possibly also completely undefine), then should we really keep referring to it in examples? As opposed to changing the examples to reflect how one wold use these instead, i.e.
- either add
K = algebraic_closure(QQ)at the start then and then useKinstead ofQQBar, or - explicitly do
QQBar = algebraic_closure(QQ)at the start of the example, or - use
QQBarFieldElem(x)instead ofQQBar(x)
There was a problem hiding this comment.
Just wanting to mention here that the same reasoning should be applied to all of the setup blocks where we create RR = RealField() and CC = ComplexField() etc.
test/calcium/qqbar-test.jl
Outdated
|
|
||
| u = sqrt(R(2)) | ||
| i = sqrt(R(-1)) | ||
| QQBar = algebraic_closure(QQ) |
There was a problem hiding this comment.
If this was just
| QQBar = algebraic_closure(QQ) | |
| R = algebraic_closure(QQ) |
the diff would be much smaller
docs/src/algebraic.md
Outdated
| associated field of algebraic numbers is represented by the constant | ||
| parent object called `CalciumQQBar`. | ||
| associated field of algebraic numbers can be constructed using | ||
| `QQBar = algebraic_closure(QQ)`. |
There was a problem hiding this comment.
| `QQBar = algebraic_closure(QQ)`. | |
| `algebraic_closure(QQ)`. |
|
I think |
| @@ -38,8 +28,7 @@ For fast calculation in $\overline{\mathbb{Q}}$, | |||
| `CalciumField` should typically be used instead (see the section | |||
| on *Exact real and complex numbers*). | |||
| Alternatively, to compute in a fixed subfield of $\overline{\mathbb{Q}}$, | |||
| you may fix a generator $a$ and construct an | |||
| Antic number field to represent $\mathbb{Q}(a)$. | |||
| you may fix a generator $a$ and construct a number field to represent $\mathbb{Q}(a)$. | |||
There was a problem hiding this comment.
I realize you are just editing existing text, so this is not a change request for this PR, but I genuinely wonder how to read this. Is a supposed to be an element of CalciumField ?! But in general were would a come from? Shouldn't we just say something like
Alternatively, one can often work in a suitable number field created via
number_field.
| Calcium | `QQBarFieldElem` | `QQBarField` | ||
| Library | Element type | Parent type | ||
| ----------------|------------------|-------------------- | ||
| Calcium | `QQBarFieldElem` | `QQBarField` |
There was a problem hiding this comment.
Again not part of this PR, but should we really still refer to "Calcium" here (and arb elsewhere), given that they have all been merged into FLINT?
test/calcium/qqbar-test.jl
Outdated
| @test base_ring(R) == R | ||
| @test base_ring(QQBarFieldElem(3)) == R |
There was a problem hiding this comment.
Huh, why did the base_ring change?
test/calcium/qqbar-test.jl
Outdated
|
|
||
| u = sqrt(R(2)) | ||
| i = sqrt(R(-1)) | ||
| QQBar = algebraic_closure(QQ) |
There was a problem hiding this comment.
Why not
| QQBar = algebraic_closure(QQ) | |
| R = algebraic_closure(QQ) |
and then have the rest of this test stay unchanged?
There was a problem hiding this comment.
I've tweaked this PR a bit and now I am happy with it in its current state, but now @lgoettgens should verify he is, too ;-)
| @@ -86,8 +86,6 @@ Root 0.866025 of 4x^2 - 3 | |||
| struct QQBarField <: Field | |||
| end | |||
|
|
|||
| const QQBar = QQBarField() | |||
|
|
|||
| @doc qq_field_doc | |||
There was a problem hiding this comment.
Off-topic, but this is wrong. It should attach qqbar_field_doc instead
|
Thanks @fingolfin for bringing this pr to this form. I am happy to have it merged |
Some more progress towards oscar-system/Oscar.jl#3416.
This follows the approach of oscar-system/Oscar.jl#3416 (comment), i.e.:
CalciumQQBar(global constant)QQBar(global constant)algebraic_closure(QQ)instead.In the next breaking release of Nemo, a few things marked with a
TODOcan be removed. This will only be a breaking change for Nemo, but not for Oscar.