Add docstrings for ZZ, QQ, QQBar, algebraic_closure #1708
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1708 +/- ##
==========================================
- Coverage 84.93% 84.76% -0.18%
==========================================
Files 95 95
Lines 37235 37216 -19
==========================================
- Hits 31627 31546 -81
- Misses 5608 5670 +62 ☔ View full report in Codecov by Sentry. |
src/calcium/CalciumTypes.jl
Outdated
| QQBarFieldElem <: FieldElem | ||
|
|
||
| The algebraic closure $\overline{\mathbb Q}$ of the rationals $\mathbb Q$ and its elements. | ||
| For convenience, we predefine the global variable `const QQBar = QQBarField()`. |
There was a problem hiding this comment.
I don't know if this confuses users, as this global does not exist in Oscar. Maybe it's better to advertise algebraic_closure(QQ) or write Nemo.QQBar.
There was a problem hiding this comment.
Do we have algebraic_closure(QQ) in Nemo?
There was a problem hiding this comment.
Line 1533 in 928e06a
There was a problem hiding this comment.
Then I suggest we change the docstring as you suggested
fingolfin
force-pushed
the
mh/docstrings
branch
from
bde412f
to
42baf2d
Compare
|
OK to merge this now? |
| julia> sqrt(K(2)) | ||
| Root 1.41421 of x^2 - 2 | ||
|
|
||
| julia> QQBar(2//3) |
There was a problem hiding this comment.
this still mentiones the singleton QQBar in this example (and three rows below). x-ref #1715
| julia> K = algebraic_closure(QQ) | ||
| Field of algebraic numbers | ||
|
|
||
| julia> sqrt(K(2)) |
There was a problem hiding this comment.
| julia> K = algebraic_closure(QQ) | |
| Field of algebraic numbers | |
| julia> sqrt(K(2)) | |
| julia> QQBar = algebraic_closure(QQ) | |
| Field of algebraic numbers | |
| julia> sqrt(QQBar(2)) |
this would be one workaround to not use the global object QQBar in docstrings. https://github.com/Nemocas/Nemo.jl/pull/1708/files#r1568401981 then becomes obsolete
| julia> K = algebraic_closure(QQ) | ||
| Field of algebraic numbers | ||
|
|
||
| julia> sqrt(K(2)) |
There was a problem hiding this comment.
| julia> K = algebraic_closure(QQ) | |
| Field of algebraic numbers | |
| julia> sqrt(K(2)) | |
| julia> QQBar = algebraic_closure(QQ) | |
| Field of algebraic numbers | |
| julia> sqrt(QQBar(2)) |
maybe change here as well for consistency
There was a problem hiding this comment.
I really don't like using QQBar like this, I wouldn't use it like that myself -- I'd rather use a "realistic" example, with a name like K. (besides, wouldn't that break CI if I did it now, while Nemo still provides and exports QQBar).
|
I am merging this as is now: the QQBar removal is a separate thing, this PR on its own adds some value and I want to get it out of my hair. |
Resolves #1680