-
-
Notifications
You must be signed in to change notification settings - Fork 654
Completion of polynomial rings and their fraction fields #40635
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
base: develop
Are you sure you want to change the base?
Conversation
Documentation preview for this PR (built with commit 973dffa; changes) is ready! 🎉 |
@fchapoton @mantepse @tscrim
Of course, any other comment is welcome! |
from sage.rings.laurent_series_ring import LaurentSeriesRing | ||
|
||
|
||
class MorphismToCompletion(RingHomomorphism): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels oddly specific? Can sage/rings/polynomial/polynomial_ring_homomorphism.pyx
be reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably right. I will try to refactor things differently.
@@ -392,7 +392,7 @@ def PowerSeriesRing(base_ring, name=None, arg2=None, names=None, | |||
|
|||
# the following is the original, univariate-only code | |||
|
|||
if isinstance(name, (int, integer.Integer)): | |||
if isinstance(name, (int, integer.Integer)) or name is infinity: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the comment above is not quite true? Also while there's the opportunity consider seeing if it's possible to avoid code copy paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't really understand your point. What changes do you propose?
@@ -370,13 +370,37 @@ cdef class CategoryObject(SageObject): | |||
sage: B.<z> = EquationOrder(x^2 + 3) # needs sage.rings.number_field | |||
sage: z.minpoly() # needs sage.rings.number_field | |||
x^2 + 3 | |||
|
|||
If the ring has less than `n` generators, the generators | |||
of the base rings are happened recursively:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean "are appended".
Wait, why are we doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise
sage: A.<x> = QQ[]
sage: B.<u,a> = A.completion(x^2 + x + 1)
does not work because B._first_ngens(2)
just returns u
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explain it in the documentation instead of here, I guess. But are you sure you don't want to just redefine _defining_names
of the responsible class to return two objects instead?
Also, what are u
and a
here? If u = B(x^2+x+1)
and a = B(x)
(or vice versa) then… hm…
one can argue that B(x)
alone generates the whole of B
, so we could left that one alone and make an additional method that returns just the other item and assign that one separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explain it in the documentation instead of here, I guess. But are you sure you don't want to just redefine
_defining_names
of the responsible class to return two objects instead?
Well, the responsible class is PowerSeriesRing
which is widely used, I'm not sure it's a good idea to change the behavior of _defining_names
. (I agree that _first_ngens
is also widely used, but it looks safer to me because we give the length of the list as an argument.)
Also, what are
u
anda
here? Ifu = B(x^2+x+1)
anda = B(x)
(or vice versa) then… hm…
The completion is a ring of power series over a certain extension of the base ring; in my example, this extension is the number field
On the other hand,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So,
$A = ℚ[x]$ - natural quotient map
$A → k$ where$k = ℚ[x]/(x^2+x+1)$ sending$x$ to$a$ (here we introduce$a$ ) -
$B = k〚u〛$ (here we introduce$u$ ) - natural injection
$ℚ[x]/(x^2+x+1) → B$ - composing the two sends
$A[x]$ to$B$ , the map has kernel$(x^2+x+1)$
doesn't sound right to me. I'd guess
I'd say making a coercion is fine, but shouldn't this already exist as the composition of the two maps above? (does Sage figure things out by itself?)
Anyway typing an additional line B.<u,a>
it isn't entirely clear which is which, so I don't see this (potentially breaking) change too useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So,
$A = ℚ[x]$ - natural quotient map
$A → k$ where$k = ℚ[x]/(x^2+x+1)$ sending$x$ to$a$ (here we introduce$a$ )$B = k[[u]]$ (here we introduce$u$ )- natural injection
$ℚ[x]/(x^2+x+1) → B$ - composing the two sends
$A[x]$ to$B$ , the map has kernel$(x^2+x+1)$ doesn't sound right to me. I'd guess
$A(x^2 + x + 1)$ to be$u$ , not zero.
The inclusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually another option to define a map that takes
Because we need to solve the equation
INPUT: | ||
|
||
- ``p`` (default: ``None``) -- an irreduclible polynomial or | ||
``Infiniity``; if ``None``, the generator of this polynomial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, too many i
in Infiniity
The more I think about it, the more I am convinced that completion deserves their own class. Indeed, we would like some methods available, e.g. The reason why I rejected this option at first is that it somehow breaks the unicity assumption for parents, in the sense that I will need to include
although So I finally think that having two different copies of What do you think about this? |
okay, makes sense. In that case, I suggest design the API as follows. internally elements of the above are what you did. The below aren't. We must not expose the internal implementation detail above to the user.
Anyway, the above would require making separate parent and element classes. |
I am not really in favor to choose I'm not also sure that |
I invite @roed314 to the discussion; he might have good suggestions. |
I think there was a typo somewhere. Let You may send (
I guess you can give the user
No solving system of equation needed either way. [edit: okay I guess Witt vectors display format may be more convenient in some settings, but mimicking the current behavior of Zp is more elementary, and you don't have to choose a generator name.]
The element [sorry, mistake. I guess you can provide a method for this, such thing would have representation like
Same thing happens in quotient of polynomial ring, e.g. in If the user wants some letter, I guess one can do something similar to |
Let me summarize because I'm getting lost. Let However, it turns out that In what follows, I consider two of them. Denoting by My proposal is that I'm not sure what's your proposal. My guess is that you would like to carry computations in Of course, another different option would be to just implement operations in |
Oh, I just noticed that computing Let me also mention that in the |
I will try to take a look at the code and discussion this evening. |
Quick comments:
|
I just pushed a rough implementation (without doctest so far) using Here is a short demo:
We can also recover the underlying power series ring as follows:
However, the construction |
Finally, I used the method
|
Looks nice, though I haven't looked too carefully. I wonder what's a good way out of this. Maybe refactor the code such that
desugar to
then Caveat being currently Maybe
|
A question: each time, I change one line in a |
Yes, this is terrible. This happens for everybody. This is supposed to be cured by the switch to meson in #39030 |
you can already use meson using conda/mamba (I recommend mamba as it's faster to install). See https://doc.sagemath.org/html/en/installation//meson.html Anyway, I think |
Il faut pas de return dans une methode |
@@ -2777,9 +2777,9 @@ def commutes(self, other): | |||
functors in opposite order works. It does:: | |||
|
|||
sage: P.<x> = ZZ[] | |||
sage: C = P.completion(x).construction()[0] | |||
sage: C = P.completion('x').construction()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does P.completion(x)
no longer work? It ought to work right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.completion(x)
works but it now returns a CompletionPolynomialRing
.
This doctest actually fails without the modification but IMO, it is the fault of the doctest that pretends that completion commutes with localization, which is not true.
In this precise example, for example, the fraction field of the completion of
@@ -2541,7 +2541,7 @@ def __init__(self, p, prec, extras=None): | |||
from sage.rings.infinity import Infinity | |||
if self.p == Infinity: | |||
if self.type not in self._real_types: | |||
raise ValueError("completion type must be one of %s" % (", ".join(self._real_types))) | |||
raise ValueError("completion type must be one of %s" % (", ".join(self._real_types[1:]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can the type be None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When called with a polynomial ring and Infinity
(which was not implemented beforehand).
if x.denominator().is_one(): | ||
x = x.numerator() | ||
elif x.numerator().is_one(): | ||
x = x.denominator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels fishy, what if the element is like (x^2+1)^(-2)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the numerator is
The point is that, if we have at the same time a numerator and a denominator, then it's not a valid completion.
raise NotImplementedError("completion only currently defined " | ||
"for the maximal ideal (x)") | ||
p, n = x.perfect_power() | ||
C = A.completion(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test where p
is not a perfect power of an irreducible and ensure that an error is raised.
Maybe also add a test where elements from two different parents are added, make sure they are not implicitly coercible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, for functoriality, it is nice to have completion at nonmaximal ideals.
|
||
def teichmuller_lift(self): | ||
r""" | ||
Return the Teichmüller representative of this element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it makes sense to let this method be called on arbitrary element of Ap
e.g. Ap((x^2+x+1) + x).teichmuller_lift()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say yes: any element has a Teichmüller lift.
|
||
If the parent of this element does not contain elements | ||
of negative valuation, the expansion starts at `0`; | ||
otherwise, it starts at the valuation of the elment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, "el[e]ment".
Also what does expansion()
do for p-adic and Laurent series at the moment? This feels strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the same behavior for the start_val
to change the behavior).
expansion
is not implemented for power series ring.
for e, m in self.degree().factor(): | ||
for _ in range(m): | ||
try: | ||
f = f.nth_root(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear this algorithm is correct. For example if self = x^4
, you would try to first self.nth_root(2)
, which for all you know might give -x^2
, then you run nth_root(2)
again, if the base field is ℚ then there wouldn't be a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point! Thanks!
@@ -434,7 +434,7 @@ cdef class PowerSeries_pari(PowerSeries): | |||
if a.valuation() <= 0: | |||
raise ValueError("can only substitute elements of positive valuation") | |||
elif isinstance(Q, PolynomialRing_generic): | |||
Q = Q.completion(Q.gen()) | |||
Q = Q.completion(Q.variable_name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this can be seen as a breaking change (previously it returns a power series, now it returns a CompletionPolynomialRing
). The alternative is to special case such as if p == self.gen():
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No: Q.completion('x')
(with a string) still returns a PowerSeriesRing
; it's useful in particular for functoriality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I would special case if the generator is the input.
@@ -593,7 +594,7 @@ cdef class RingExtension_generic(Parent): | |||
# Some checks | |||
if (base not in CommutativeRings() | |||
or ring not in CommutativeRings() | |||
or not defining_morphism.category_for().is_subcategory(CommutativeRings())): | |||
or not defining_morphism.category_for().is_subcategory(Rings())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I was not able to understand, a certain coercion morphism (or maybe a composition of two of them) is in the category of Rings
and not CommutativeRings
.
Probably, this should be fixed but, in any case, since any ring morphism between commutative rings is certainly a morphism of commutative rings, I think it's very safe to do this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definitely seems like a bug and deserves its own issue (which should also be referenced here I think).
Okay, thank you for the summary. As a reminder |
@@ -593,7 +594,7 @@ cdef class RingExtension_generic(Parent): | |||
# Some checks | |||
if (base not in CommutativeRings() | |||
or ring not in CommutativeRings() | |||
or not defining_morphism.category_for().is_subcategory(CommutativeRings())): | |||
or not defining_morphism.category_for().is_subcategory(Rings())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definitely seems like a bug and deserves its own issue (which should also be referenced here I think).
with the construction mecanism | ||
|
||
EXAMPLES:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the construction mecanism | |
EXAMPLES:: | |
with the construction mechanism | |
.. SEEALSO:: | |
:mod:`sage.rings.completion_polynomial_ring` | |
EXAMPLES:: |
@@ -221,12 +221,14 @@ def __classcall__(cls, *args, **kwds): | |||
'q' | |||
""" | |||
from .power_series_ring import PowerSeriesRing | |||
|
|||
if 'default_prec' in kwds and kwds['default_prec'] is infinity: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 'default_prec' in kwds and kwds['default_prec'] is infinity: | |
if kwds.get('default_prec', None) is infinity: |
Personal style choice here.
previous variable names | ||
|
||
EXAMPLES:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previous variable names | |
EXAMPLES:: | |
previous variable names | |
.. SEEALSO:: | |
:mod:`sage.rings.completion_polynomial_ring` | |
EXAMPLES:: |
Completion of polynomial rings and their fraction fields | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completion of polynomial rings and their fraction fields | |
""" | |
Completion of polynomial rings and their fraction fields | |
AUTHORS: | |
- Xavier Caruso ([INSERT A DATE]): Initial implementation | |
""" |
Completion of Univariate Polynomial Ring in x over Integer Ring at x | ||
""" | ||
if isinstance(ring, PolynomialRing_generic): | ||
if isinstance(p, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isinstance(p, str): | |
if isinstance(p, str) or p == ring.gen(): |
As mentioned on another comment, I think it is good to special case this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, I think it is important to have something uniform (with the same parent, the same class) when the ideal varies.
It could be a mess if you do
p = <complicated calculation>
Ap = A.completion(p)
and the result you get depends on the fact that the result of the complicated calculation is or is not the generator.
Another issue concerns inexact base rings when any approximation of a generator, in particular O(1)*x
will be recognized a generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then power series rings should be subclasses. The point is that there we can take advantage of the specific input, so we can and should optimize it (and have other things like natural coercions). Although as long as the API is the same, ducktyping will make it okay, but this would be a code smell to me. Also the output type depends on the precision, so there might be a bigger refactoring needed...
""" | ||
if isinstance(ring, PolynomialRing_generic): | ||
if isinstance(p, str): | ||
return PowerSeriesRing(ring.base_ring(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need to handle the case of default_prec == oo
here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's done in the function PowerSeriesRing
.
A = ring | ||
elif isinstance(ring, FractionField_1poly_field): | ||
if isinstance(p, str): | ||
return LaurentSeriesRing(ring.base_ring(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this be wrong if the base ring is a ring? For example,
def laurent_series_ring(self, names=None, sparse=None): | ||
r""" | ||
Return a Laurent series ring, which is isomorphic to | ||
the fraction field of this completion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think we need to be a bit careful with the base ring.
def is_integral_domain(self): | ||
return self._residue_ring.is_integral_domain() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another doctest reminder.
We implement the completion of polynomial rings and their fraction fields.
This results in rings of power/Laurent series.
📝 Checklist