Opened 8 years ago
Closed 21 months ago
#18416 closed defect (fixed)
UniqueRepresentation issue with PowerSeriesRing
Reported by:  Vincent Delecroix  Owned by:  

Priority:  critical  Milestone:  sage9.3 
Component:  algebra  Keywords:  power_series 
Cc:  Niles Johnson, Simon King, Travis Scrimshaw, Samuel Lelièvre  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  f38b78e (Commits, GitHub, GitLab)  Commit:  f38b78ef74405164bb576112f4b17b6e90277c7f 
Dependencies:  #31263  Stopgaps: 
Description (last modified by )
The power series ring (sage.rings.power_series_ring.PowerSeriesRing
) inherits from UniqueRepresentation
. In the argument of the constructor there is the precision (prec
) but it can be modified! Which leads to wrong behavior
sage: R = PowerSeriesRing(QQ, 'x', 3) sage: R.default_prec() 3 sage: R.set_default_prec(19) sage: PowerSeriesRing(QQ, 'x', 3).default_prec() 19
Moreover, a function can modify a power series in use
sage: def haha() ....: PowerSeriesRing(QQ, 'x').set_default_prec(1) sage: R = PowerSeriesRing(QQ, 'x') sage: R.default_prec() 20 sage: haha() sage: R.default_prec() 1
Change History (52)
comment:1 Changed 8 years ago by
Description:  modified (diff) 

comment:2 Changed 8 years ago by
Cc:  Niles Johnson Simon King added 

comment:3 Changed 4 years ago by
Milestone:  sage6.7 → sage8.7 

comment:4 Changed 4 years ago by
+1 on deprecating the set_default_prec
for PowerSeriesRing
for the same reasons as for LaurentSeriesRing
.
comment:5 Changed 4 years ago by
Even worse in terms of hidden side effects:
sage: R.<x> = LaurentSeriesRing(QQ,50) sage: R.default_prec() 50 sage: S = PowerSeriesRing(QQ,50,names='x') sage: S.set_default_prec(10) sage: R.default_prec() 10
comment:7 Changed 4 years ago by
Milestone:  sage8.7 → sage8.8 

Moving all blocker/critical issues from 8.7 to 8.8.
comment:8 Changed 3 years ago by
Milestone:  sage8.8 → sage8.9 

Moving open critical and blocker issues to the next release milestone (optimistically).
comment:9 Changed 3 years ago by
Milestone:  sage8.9 → sage9.1 

Ticket retargeted after milestone closed
comment:10 Changed 3 years ago by
Milestone:  sage9.1 → sage9.2 

comment:11 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:12 followup: 18 Changed 23 months ago by
I propose to go back to the idea from #16201. In particular, we should handle the whole precision business using a global variable. If desired, one could introduce several global variables (e.g. one for univariate power series and another for multivariate).
I'll upload a branch soon.
comment:13 Changed 23 months ago by
Cc:  Travis Scrimshaw added 

comment:14 Changed 23 months ago by
Okay, this is tricky: mostly all doctests heavily rely on the default_prec
syntax, which is actually bad. Either we add after each example where the precision has been changed a
sage: set_series_precision(20) # reset to default
or we have to establish each example with the default precision.
At least, this behavior would be expected with the proposed changes, whereas now it is entirely random to the user.
Alternatively, we come up with an entirely different solution. I am open for suggestions.
comment:15 Changed 23 months ago by
Branch:  → u/ghmjungmath/uniquerepresentation_issue_with_powerseriesring 

comment:16 Changed 23 months ago by
Authors:  → Michael Jung 

Cc:  Samuel Lelièvre added 
Commit:  → d4b84d092820ea4c71f3925c8f09e6fbdfe142aa 
Keywords:  power_series added 
So, this my first approach. Since the global variable series_prec
keeps track of the multivariate case too (otherwise one runs into problems with the background power series ring), I set the default to 15, as a compromise for now.
If this meets your approval, we have to think what we should do with the failed doctests.
I know, this is a big change. But the current behavior should not be further promoted either.
New commits:
d4b84d0  Trac #18416: global precision

comment:17 Changed 23 months ago by
Commit:  d4b84d092820ea4c71f3925c8f09e6fbdfe142aa → 92f06b83c10f3a891f6b203deb4c225e6ddf14af 

Branch pushed to git repo; I updated commit sha1. New commits:
92f06b8  Trac #18416: unnecessary method overload

comment:18 followup: 19 Changed 23 months ago by
Replying to ghmjungmath:
I propose to go back to the idea from #16201. In particular, we should handle the whole precision business using a global variable. If desired, one could introduce several global variables (e.g. one for univariate power series and another for multivariate).
I am a very strong 1 on having a global variable for default precision. This would have far too many unintended side effects where code could seeminglyrandomly change because you changed the default precision. This is something that should be strongly associated to the ring. I think the idea on #16201 of removing the ability to set the default precision is the correct one. If you want to change the precision, you need a new ring. This would be in line with real numbers.
comment:19 Changed 23 months ago by
Replying to tscrim:
Replying to ghmjungmath:
I propose to go back to the idea from #16201. In particular, we should handle the whole precision business using a global variable. If desired, one could introduce several global variables (e.g. one for univariate power series and another for multivariate).
I am a very strong 1 on having a global variable for default precision. This would have far too many unintended side effects where code could seeminglyrandomly change because you changed the default precision. This is something that should be strongly associated to the ring. I think the idea on #16201 of removing the ability to set the default precision is the correct one. If you want to change the precision, you need a new ring. This would be in line with real numbers.
I totally agree: no precision should be handled by global variables. We have parents exactly for that purpose.
comment:20 Changed 23 months ago by
Watching how unpredictable the doctest behaves after this change, I totally agree. To be honest, I wasn't so convinced of this idea either.
So, remove all doctest related to set_default_prec
and deprecate that function?
comment:21 Changed 23 months ago by
What about the current global variable? I think that should be deprecated as well if we don't pursue that behavior.
comment:22 Changed 23 months ago by
For some reason, both the Laurent series ring and power series ring refer to the global series precision in the documentation even though they just use the locally defined default precision
However, effectively, the global series precision is only used in expression.pyx
.
How do we treat elements of a power series ring with different precision than its parent? I tend to say that the element's precision should be fixed and linked to the parent. But this would change the entire behavior of power series.
comment:23 followup: 24 Changed 23 months ago by
Yes, we should deprecate set_default_prec
.
There is some use for that global variable, that is to ensure consistency across implementations in Sage as it is not exposed to the user. It can also be used by an expert who knows what they are doing.
There should be a coercion from a parent with more (default) precision to one with less precision (over the same variables and base ring). (Of course, conversions should clearly be allowed.) This is in agreement with real numbers and if you create one with less precision, you are expecting to have less precision in the future. On the other hand, there is an argument that would say that because of how the precision is used, we should go the other way because we hold onto the cutoff and we should be as accurate as possible.
I doubt for most people there will be any practical change.
comment:24 followup: 25 Changed 23 months ago by
Replying to tscrim:
Yes, we should deprecate
set_default_prec
.There is some use for that global variable, that is to ensure consistency across implementations in Sage as it is not exposed to the user. It can also be used by an expert who knows what they are doing.
There should be a coercion from a parent with more (default) precision to one with less precision (over the same variables and base ring). (Of course, conversions should clearly be allowed.) This is in agreement with real numbers and if you create one with less precision, you are expecting to have less precision in the future. On the other hand, there is an argument that would say that because of how the precision is used, we should go the other way because we hold onto the cutoff and we should be as accurate as possible.
I doubt for most people there will be any practical change.
Then, this needs a change of the current behavior, which is that the element's precision and precision of its parent are independent:
sage: R.<x> = PowerSeriesRing(QQ, default_prec=10) sage: p = R(x, prec=20); p x + O(x^20) sage: p.sin() x  1/6*x^3 + 1/120*x^5  1/5040*x^7 + 1/362880*x^9  1/39916800*x^11 + 1/6227020800*x^13  1/1307674368000*x^15 + 1/355687428096000*x^17  1/121645100408832000*x^19 + O(x^20) sage: p.sin() in R True
comment:25 Changed 23 months ago by
Replying to ghmjungmath:
Then, this needs a change of the current behavior, which is that the element's precision and precision of its parent are independent
There's no particular problem with that: it's a default precision. If you explicitly want, you can ask for more precision. The issue arises because some operations (like coercing 1/(1+x) into a power series ring) need to choose a finite precision. In those cases, the default precision comes in.
There are other models; see the padics: for computational efficiency you may want to ensure you're never carrying around *more* than a certain number of terms. That would lead to a "capped precision" ring. But then you probably still want to allow for elements with less precision, because cancellation may mean that you simply can't determine more than a certain number of terms.
comment:26 followup: 28 Changed 23 months ago by
I agree in so far as currently there's no particular problem with that behavior. In fact, the term default_prec
and especially the method set_defeault_prec
even promote it.
However, if we want to fix the (default) precision of a power series ring instance, as Travis suggested in line with the real numbers, wouldn't it be confusing if its elements can still adopt higher (or lower) precision? This could also lead to unexpected and unwanted behavior when you try to coerce elements into elements of rings with higher/lower precision. This is in line with the real numbers, where you choose a precision and cannot increase the precision afterwards, and this makes perfect sense for power series, too. Increasing the precision (however that shall be possible after we've fixed the default precision and cannot be changed anymore), should then lead to another parent with corresponding higher precision.
One exception could be an element with infinite precision (i.e. a polynomial).
comment:27 Changed 23 months ago by
I think you are again confusing the precision of the element and the default precision of the ring. The former doesn't change no matter the default precision of the ring you are in. The latter only comes into play when you are doing a computation that requires a cutoff, such as division. The reality is I doubt anyone will be making a bunch of different power series rings with different precisions and mixing them together. The analog I made with the real numbers is not an exact comparison, but merely to point out the precedence for having a precision value set in the parent construction.
comment:28 followup: 29 Changed 23 months ago by
Replying to ghmjungmath:
This is in line with the real numbers, where you choose a precision and cannot increase the precision afterwards, and this makes perfect sense for power series, too.
The analogue of floating point numbers would be a laurent polynomial ring where you just chop after a fixed number of terms. Power series rings and padics actually more resemble the RealIntervalField
and RealBallField
, where elements carry precision information with it, so that you can actually PROVE that two elements are distinct (you can only do that with floating point if you analyze the computation that you have used to arrive at those numbers).
comment:29 Changed 23 months ago by
Okay, I obviously misunderstood Travis' comparison with the real numbers. Thank you both for your thorough explaination.
So, the idea is then simply to deprecate set_default_prec
, remove its use in the documentation and provide a coercion (and keep the precision for elements)? Then add some examples like
sage: R.<x> = PowerSeriesRing(QQ, default_prec=10) sage: f = sin(x); f x  1/6*x^3 + 1/120*x^5  1/5040*x^7 + 1/362880*x^9 + O(x^10) sage: P.<x> = PowerSeriesRing(QQ, default_prec=5) sage. P(f) x  1/6*x^3 + O(x^5)
comment:30 Changed 22 months ago by
Yes, that is correct other than the example, where the result of P(f)
will be the same f
. However, a good illustration of the difference is doing the exact same computation but in P
(which will yield the output you have).
comment:31 Changed 22 months ago by
Commit:  92f06b83c10f3a891f6b203deb4c225e6ddf14af → cdb5c553a4203d963b4a148f8d4c344b0f239685 

comment:32 Changed 22 months ago by
Status:  new → needs_review 

comment:34 Changed 22 months ago by
Commit:  cdb5c553a4203d963b4a148f8d4c344b0f239685 → 78cef3829c679fc857be02232023cf6bd7d743f1 

Branch pushed to git repo; I updated commit sha1. New commits:
78cef38  Trac #18416: remove set_default_prec in source code

comment:35 Changed 22 months ago by
Overall yes, but I think it would be better to deprecate the set_default_prec
in Nonexact
and continue to use that mixin class. It is a nice indicator class that might be useful. Plus it would fix both types of power series rings in one place and make future maintenance easier.
comment:36 Changed 22 months ago by
Are you sure this is a good idea? I mean, this method only causes trouble when you mixin the class together with UniqueRepresentation
. What if one still wants to use Nonexact
and the feature set_default_prec
?
comment:37 Changed 22 months ago by
Commit:  78cef3829c679fc857be02232023cf6bd7d743f1 → a13c408db8ab421b8ab0fa3cf4d43babe78be33d 

Branch pushed to git repo; I updated commit sha1. New commits:
a13c408  Trac #18416: Nonexact improved + set_default_prec deprecated

comment:38 Changed 22 months ago by
Okay, this class is used nowhere else. Instead, I have deprecated the method in Nonexact
and improved its documentation to clarify the proper usage.
comment:39 Changed 22 months ago by
Commit:  a13c408db8ab421b8ab0fa3cf4d43babe78be33d → f806db1621786a5c6488ee72f1d5e8742d1e0d60 

Branch pushed to git repo; I updated commit sha1. New commits:
f806db1  Trac #18416: remove double underscore;__default_prec > _default_prec

comment:40 Changed 22 months ago by
The only things I would change before giving a positive review (with a green patchbot) are the deprecation message to
The method set_default_prec() is deprecated and will be removed in a future version of Sage. The default precision is set during construction.
and this
The default precision of a power series ring instance stays fixed and cannot be changed. To work with different default precisions, we must establish new instances instead:: +The default precision of a power series ring stays fixed and +cannot be changed. To work with different default precision, create +a new power series ring::
comment:41 Changed 22 months ago by
Commit:  f806db1621786a5c6488ee72f1d5e8742d1e0d60 → af4280479548603e4b0cab8cf81047eba8a4ab0c 

comment:45 Changed 22 months ago by
Dependencies:  → #31263 

 you could have used "make build" instead of "sage b", until this is repaired
 do not rebase, but add #31263 in the dependency field here above (I did that for you)
 changing the src/bin/sage file make your ticket "unsafe", so patchbots do not test it
comment:46 Changed 22 months ago by
That is good to know. Thank you for pointing this out.
Travis, are you satisfied?
comment:47 Changed 22 months ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → positive_review 
Yep. LGTM. Thank you.
comment:48 Changed 22 months ago by
Status:  positive_review → needs_work 

[dochtml] Traceback (most recent call last): [dochtml] File "/usr/lib64/python3.9/runpy.py", line 197, in _run_module_as_main [dochtml] return _run_code(code, main_globals, None, [dochtml] File "/usr/lib64/python3.9/runpy.py", line 87, in _run_code [dochtml] exec(code, run_globals) [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__main__.py", line 2, in <module> [dochtml] main() [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__init__.py", line 1738, in main [dochtml] builder() [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__init__.py", line 345, in _wrapper [dochtml] getattr(get_builder(document), 'inventory')(*args, **kwds) [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__init__.py", line 577, in _wrapper [dochtml] self._build_everything_except_bibliography(lang, format, *args, **kwds) [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__init__.py", line 563, in _build_everything_except_bibliography [dochtml] build_many(build_ref_doc, non_references) [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__init__.py", line 297, in build_many [dochtml] _build_many(target, args, processes=processes) [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/utils.py", line 289, in build_many [dochtml] raise worker_exc.original_exception [dochtml] OSError: /home/release/Sage/local/lib64/python3.9/sitepackages/sage/structure/nonexact.py:docstring of sage.structure.nonexact.Nonexact:6: WARNING: Bullet list ends without a blank line; unexpected unindent. [dochtml] [dochtml] Note: incremental documentation builds sometimes cause spurious [dochtml] error messages. To be certain that these are real errors, run [dochtml] "make docclean" first and try again. make[3]: *** [Makefile:1916: dochtml] Error 1
comment:49 Changed 22 months ago by
Commit:  af4280479548603e4b0cab8cf81047eba8a4ab0c → f38b78ef74405164bb576112f4b17b6e90277c7f 

Branch pushed to git repo; I updated commit sha1. New commits:
f38b78e  Trac #18416: docstring bullet point

comment:50 Changed 22 months ago by
Status:  needs_work → needs_review 

Just one character missing...here we go.
comment:51 Changed 22 months ago by
Status:  needs_review → positive_review 

comment:52 Changed 21 months ago by
Branch:  u/ghmjungmath/uniquerepresentation_issue_with_powerseriesring → f38b78ef74405164bb576112f4b17b6e90277c7f 

Resolution:  → fixed 
Status:  positive_review → closed 
Does anyone know the status of this?
The corresponding behavior for LaurentSeries? was deprecated in #16201 (which also mentions power series) and finally removed in #26915 .
So now we are in the situation where PowerSeries? and LaurentSeries? work quite differently here, I can't see any real reason for this.