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: sage-9.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:

Status badges

Description (last modified by Vincent Delecroix)

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 Vincent Delecroix

Description: modified (diff)

comment:2 Changed 8 years ago by Vincent Delecroix

Cc: Niles Johnson Simon King added

comment:3 Changed 4 years ago by Alex J. Best

Milestone: sage-6.7sage-8.7

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.

comment:4 Changed 4 years ago by Travis Scrimshaw

+1 on deprecating the set_default_prec for PowerSeriesRing for the same reasons as for LaurentSeriesRing.

comment:5 Changed 4 years ago by Travis Scrimshaw

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:6 Changed 4 years ago by Niles Johnson

yes, this should definitely be changed

comment:7 Changed 4 years ago by Erik Bray

Milestone: sage-8.7sage-8.8

Moving all blocker/critical issues from 8.7 to 8.8.

comment:8 Changed 3 years ago by Erik Bray

Milestone: sage-8.8sage-8.9

Moving open critical and blocker issues to the next release milestone (optimistically).

comment:9 Changed 3 years ago by Erik Bray

Milestone: sage-8.9sage-9.1

Ticket retargeted after milestone closed

comment:10 Changed 3 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.2

comment:11 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:12 Changed 23 months ago by Michael Jung

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 Michael Jung

Cc: Travis Scrimshaw added

comment:14 Changed 23 months ago by Michael Jung

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 Michael Jung

Branch: u/gh-mjungmath/uniquerepresentation_issue_with_powerseriesring

comment:16 Changed 23 months ago by Michael Jung

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:

d4b84d0Trac #18416: global precision

comment:17 Changed 23 months ago by git

Commit: d4b84d092820ea4c71f3925c8f09e6fbdfe142aa92f06b83c10f3a891f6b203deb4c225e6ddf14af

Branch pushed to git repo; I updated commit sha1. New commits:

92f06b8Trac #18416: unnecessary method overload

comment:18 in reply to:  12 ; Changed 23 months ago by Travis Scrimshaw

Replying to gh-mjungmath:

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 seemingly-randomly 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 in reply to:  18 Changed 23 months ago by Vincent Delecroix

Replying to tscrim:

Replying to gh-mjungmath:

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 seemingly-randomly 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 Michael Jung

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 Michael Jung

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 Michael Jung

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.

Last edited 23 months ago by Michael Jung (previous) (diff)

comment:23 Changed 23 months ago by Travis Scrimshaw

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 in reply to:  23 ; Changed 23 months ago by Michael Jung

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 in reply to:  24 Changed 23 months ago by Nils Bruin

Replying to gh-mjungmath:

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 p-adics: 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 Changed 23 months ago by Michael Jung

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).

Last edited 23 months ago by Michael Jung (previous) (diff)

comment:27 Changed 23 months ago by Travis Scrimshaw

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 in reply to:  26 ; Changed 23 months ago by Nils Bruin

Replying to gh-mjungmath:

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 p-adics 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 in reply to:  28 Changed 23 months ago by Michael Jung

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)
Last edited 22 months ago by Michael Jung (previous) (diff)

comment:30 Changed 22 months ago by Travis Scrimshaw

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 git

Commit: 92f06b83c10f3a891f6b203deb4c225e6ddf14afcdb5c553a4203d963b4a148f8d4c344b0f239685

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fec4b06fix sage -b after 30622
dff846cgo through make/build/install
4fe2c12Trac #18416: deprecate set_default_prec
cdb5c55Trac #18416: add doctest

comment:32 Changed 22 months ago by Michael Jung

Status: newneeds_review

comment:33 Changed 22 months ago by Michael Jung

Something like this?

comment:34 Changed 22 months ago by git

Commit: cdb5c553a4203d963b4a148f8d4c344b0f23968578cef3829c679fc857be02232023cf6bd7d743f1

Branch pushed to git repo; I updated commit sha1. New commits:

78cef38Trac #18416: remove set_default_prec in source code

comment:35 Changed 22 months ago by Travis Scrimshaw

Overall yes, but I think it would be better to deprecate the set_default_prec in Nonexact and continue to use that mix-in 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 Michael Jung

Are you sure this is a good idea? I mean, this method only causes trouble when you mix-in the class together with UniqueRepresentation. What if one still wants to use Nonexact and the feature set_default_prec?

Last edited 22 months ago by Michael Jung (previous) (diff)

comment:37 Changed 22 months ago by git

Commit: 78cef3829c679fc857be02232023cf6bd7d743f1a13c408db8ab421b8ab0fa3cf4d43babe78be33d

Branch pushed to git repo; I updated commit sha1. New commits:

a13c408Trac #18416: Nonexact improved + set_default_prec deprecated

comment:38 Changed 22 months ago by Michael Jung

Okay, this class is used nowhere else. Instead, I have deprecated the method in Nonexact and improved its documentation to clarify the proper usage.

Last edited 22 months ago by Michael Jung (previous) (diff)

comment:39 Changed 22 months ago by git

Commit: a13c408db8ab421b8ab0fa3cf4d43babe78be33df806db1621786a5c6488ee72f1d5e8742d1e0d60

Branch pushed to git repo; I updated commit sha1. New commits:

f806db1Trac #18416: remove double underscore;__default_prec -> _default_prec

comment:40 Changed 22 months ago by Travis Scrimshaw

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::
Last edited 22 months ago by Travis Scrimshaw (previous) (diff)

comment:41 Changed 22 months ago by git

Commit: f806db1621786a5c6488ee72f1d5e8742d1e0d60af4280479548603e4b0cab8cf81047eba8a4ab0c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

eecbf2aTrac #18416: deprecate set_default_prec
0a176d1Trac #18416: remove double underscore;__default_prec -> _default_prec
af42804Trac #18416: deprecation message

comment:42 Changed 22 months ago by Michael Jung

There we go. :)

comment:43 Changed 22 months ago by Frédéric Chapoton

changing src/bin/sage is not a thing to do

comment:44 Changed 22 months ago by Michael Jung

I merged #31263 to use the sage -b command...

Rebase?

comment:45 Changed 22 months ago by Frédéric Chapoton

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 Michael Jung

That is good to know. Thank you for pointing this out.

Travis, are you satisfied?

comment:47 Changed 22 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Yep. LGTM. Thank you.

comment:48 Changed 22 months ago by Volker Braun

Status: positive_reviewneeds_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/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__init__.py", line 1738, in main
[dochtml]     builder()
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/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/site-packages/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 doc-clean" first and try again.
make[3]: *** [Makefile:1916: doc-html] Error 1

comment:49 Changed 22 months ago by git

Commit: af4280479548603e4b0cab8cf81047eba8a4ab0cf38b78ef74405164bb576112f4b17b6e90277c7f

Branch pushed to git repo; I updated commit sha1. New commits:

f38b78eTrac #18416: docstring bullet point

comment:50 Changed 22 months ago by Michael Jung

Status: needs_workneeds_review

Just one character missing...here we go.

comment:51 Changed 22 months ago by Travis Scrimshaw

Status: needs_reviewpositive_review

comment:52 Changed 21 months ago by Volker Braun

Branch: u/gh-mjungmath/uniquerepresentation_issue_with_powerseriesringf38b78ef74405164bb576112f4b17b6e90277c7f
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.