Opened 6 years ago

Closed 5 years ago

#16201 closed enhancement (fixed)

default precision for all series (symbolic, power, Laurent)

Reported by: rws Owned by:
Priority: major Milestone: sage-6.4
Component: algebra Keywords: series precision
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Clemens Heuberger
Report Upstream: N/A Work issues:
Branch: 0d7da1b (Commits) Commit: 0d7da1b7f690134eb9109e7062d06df59a5e3ab3
Dependencies: Stopgaps:

Description (last modified by cheuberg)

Both Laurent and power series ring elements have a default precision of 20. OTOH, symbolic series expansions need the precision set:

sage: R.<t> = LaurentSeriesRing(QQ)
sage: 1/(1-t)
1 + t + t^2 + t^3 + t^4 + t^5 + t^6 + t^7 + t^8 + t^9 + t^10 + t^11 + t^12 + t^13 + t^14 + t^15 + t^16 + t^17 + t^18 + t^19 + O(t^20)
sage: var('x')
x
sage: ex=1/(1-x)
sage: ex.series(x)
...
TypeError: series() takes exactly 2 positional arguments (1 given)

This ticket proposes to unify the behaviour by allowing the following:

sage: ex.series(x)
1 + 1*x + 1*x^2 + 1*x^3 + 1*x^4 + 1*x^5 + 1*x^6 + 1*x^7 + 1*x^8 + 1*x^9 + 1*x^10 + 1*x^11 + 1*x^12 + 1*x^13 + 1*x^14 + 1*x^15 + 1*x^16 + 1*x^17 + 1*x^18 + 1*x^19 + Order(x^20)

As this is about reducing surprise for the user both algebra and symbolic series default precision should have the same value, so the ticket should also implement such a global value.

This ticket also deprecates the use of LaurentSeriesRing_generic.set_default_prec as it contradicts the immutability of parents and conflicts with the caching of LaurentSeriesRings?.

Change History (23)

comment:1 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:2 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:3 Changed 5 years ago by rws

  • Branch set to u/rws/default_precision_for_symbolic_power_series_from_sr_series__

comment:4 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 5fc4999530c6462e560b07dcb2c99463ae6a4a55
  • Status changed from new to needs_review
  • Summary changed from default precision for symbolic power series from SR.series() to default precision for all series (symbolic, power, Laurent)

New commits:

5fc499916201: implement global default series precision, accessors

comment:5 Changed 5 years ago by cheuberg

I do not think that order=-1 is a good default value, as it changes previous behaviour:

Prior to this patch:

sage: f = sin(x)^(-2)
....: f.series(x, -1)
1*x^(-2) + Order(1/x)

After this patch:

sage: f = sin(x)^(-2)
....: f.series(x, -1)
1*x^(-2) + 1/3 + 1/15*x^2 + 2/189*x^4 + 1/675*x^6 + 2/10395*x^8 + 1382/58046625*x^10 + 4/1403325*x^12 + 3617/10854718875*x^14 + 87734/2292899734125*x^16 + 349222/80596287646875*x^18 + Order(x^20)

Furthermore, I'd like to see doctests testing the new behaviour.

Finally, sage.misc.defaults could be added to the reference manual such that the documentation of all three modified methods (symbolic, power, Laurent) could link to that place for setting the default precision.

comment:6 Changed 5 years ago by git

  • Commit changed from 5fc4999530c6462e560b07dcb2c99463ae6a4a55 to b3cb45774f2f91f75ea070ffac22a826c7cff396

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

b3cb45716201: change keyword default; add doctests; add misc.defaults to doctree and point to it

comment:7 follow-up: Changed 5 years ago by rws

Done. Unfortunately this is Cython so I had to pick a value that signals default, changed to -65535 now.

Please add your realname to Reviewers.

comment:8 in reply to: ↑ 7 Changed 5 years ago by cheuberg

  • Reviewers set to Clemens Heuberger
  • Status changed from needs_review to needs_work

Replying to rws:

Done. Unfortunately this is Cython so I had to pick a value that signals default, changed to -65535 now.

I think that replacing int order=-65535 by order=None would lead to clearer code at a negligible performance penalty.

Further comments:

  • The docstrings of series_precision and set_series_precision should have EXAMPLE sections.
  • The parameter default_prec of the Laurent series classes should be explained; the only explanation now is in def default_prec(self) which should be rephrased anyway ("Set ..." is not correct).
  • I'd like to see doctests showing examples on how to construct series with different values for set_series_precision
  • :meth:`set_series_precision` is a dead link.

comment:9 Changed 5 years ago by git

  • Commit changed from b3cb45774f2f91f75ea070ffac22a826c7cff396 to 3b682e2a637b58323979f8fca2f97e470a4c2436

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

3b682e216201: misc.defauts doctests; deprecate LaurentSeriesRing.set_default_prec; cosmetics

comment:10 follow-up: Changed 5 years ago by rws

  • Status changed from needs_work to needs_review

The default for order is now None. The misc functions now have examples. The description of def default_prec(self) was corrected. I think the LaurentSeriesRing method set_default_prec should be deprecated as is already hinted. Done.

The general improvement of Laurent series documentation is ticket #12259.

I don't think doctests showing examples on how to construct series with different values for set_series_precision should be added. I don't want to encourage frequent usage of the global setting to set single series precisions.

If the link is dead you should do make doc-clean; make doc. It will not suffice to sage -docbuild since a file was added.

comment:11 in reply to: ↑ 10 Changed 5 years ago by cheuberg

  • Status changed from needs_review to needs_work

For now, just three comments, I'll have a closer look later.

File "src/sage/symbolic/expression.pyx", line 3542, in sage.symbolic.expression.Expression.series
Failed example:
    f.series(x)
Exception raised:
    Traceback (most recent call last):
      File "/local/sage/sage-6.5.beta4/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/local/sage/sage-6.5.beta4/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.expression.Expression.series[9]>", line 1, in <module>
        f.series(x)
      File "sage/symbolic/expression.pyx", line 3584, in sage.symbolic.expression.Expression.series (build/cythonized/sage/symbolic/expression.cpp:20710)
        x = self._gobj.series(symbol0._gobj, order, 0)
    TypeError: an integer is required

The reason is that prec is set but never used.

Replying to rws:

I don't think doctests showing examples on how to construct series with different values for set_series_precision should be added. I don't want to encourage frequent usage of the global setting to set single series precisions.

I concur that we should not encourage the actual frequent usage of the global settings. However, I'd prefer it to be at least be covered by a test in a "TESTS" section, just to be sure to avoid small errors as the issue outlined above.

If the link is dead you should do make doc-clean; make doc. It will not suffice to sage -docbuild since a file was added.

I think that the problem is rather the meth instead of func.

comment:12 Changed 5 years ago by cheuberg

The deprecation of set_series_precision leads to four failing doctests elsewhere:

sage -t src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py", line 74, in sage.schemes.hyperelliptic_curves.hyperelliptic_padic_field.HyperellipticCurve_padic_field.local_analytic_interpolation
Failed example:
    x,y,z = HK.local_analytic_interpolation(P,Q)
Expected nothing
Got:
    doctest:512: DeprecationWarning: This method is deprecated.
    See http://trac.sagemath.org/16201 for details.
**********************************************************************
sage -t src/sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/hyperelliptic_generic.py", line 490, in sage.schemes.hyperelliptic_curves.hyperelliptic_generic.HyperellipticCurve_generic.local_coordinates_at_infinity
Failed example:
    x,y = H.local_coordinates_at_infinity(10)
Expected nothing
Got:
    doctest:512: DeprecationWarning: This method is deprecated.
    See http://trac.sagemath.org/16201 for details.
**********************************************************************
sage -t src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 3580, in sage.schemes.hyperelliptic_curves.monsky_washnitzer.MonskyWashnitzerDifferential.coleman_integral
Failed example:
    w.coleman_integral(P,2*P)
Expected:
    O(5^6)
Got:
    doctest:512: DeprecationWarning: This method is deprecated.
    See http://trac.sagemath.org/16201 for details.
    O(5^6)
**********************************************************************
sage -t src/sage/rings/morphism.pyx
**********************************************************************
File "src/sage/rings/morphism.pyx", line 249, in sage.rings.morphism
Failed example:
    R.set_default_prec(10)
Expected nothing
Got:
    doctest:1: DeprecationWarning: This method is deprecated.
    See http://trac.sagemath.org/16201 for details.
**********************************************************************

comment:13 Changed 5 years ago by git

  • Commit changed from 3b682e2a637b58323979f8fca2f97e470a4c2436 to 0fcce825397b78e58a13b0fa5eaa930e7bd17960

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

0fcce8216201: more doctests, fixes; remove deprecated usage

comment:14 Changed 5 years ago by rws

  • Status changed from needs_work to needs_review

All done.

comment:15 follow-up: Changed 5 years ago by cheuberg

  • Description modified (diff)
  • Status changed from needs_review to positive_review

All Doctests (make ptestlong) pass; code looks ok; thanks.

The changes in hyperelliptic_generic.py demonstrate that there is a default precision of 20, too, which is not covered by this ticket. For the ReSt? formatting errors in that file, see #17599.

comment:16 Changed 5 years ago by rws

Thanks for the review.

comment:17 in reply to: ↑ 15 Changed 5 years ago by rws

Replying to cheuberg:

The changes in hyperelliptic_generic.py demonstrate that there is a default precision of 20, too, which is not covered by this ticket.

That is now #17613.

comment:18 Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This is wrongly indented:

TESTS:

    Check if changing global series precision does it right::

It should be

TESTS:

Check if changing global series precision does it right::

comment:19 Changed 5 years ago by git

  • Commit changed from 0fcce825397b78e58a13b0fa5eaa930e7bd17960 to 0d7da1b7f690134eb9109e7062d06df59a5e3ab3

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

0d7da1b16201: fix indentation

comment:20 Changed 5 years ago by rws

  • Status changed from needs_work to needs_review

comment:21 Changed 5 years ago by rws

Jeroen, can you please have a quick look again, so that we can finish it?

comment:22 Changed 5 years ago by cheuberg

  • Status changed from needs_review to positive_review

The wrong indentation has been fixed.

comment:23 Changed 5 years ago by vbraun

  • Branch changed from u/rws/default_precision_for_symbolic_power_series_from_sr_series__ to 0d7da1b7f690134eb9109e7062d06df59a5e3ab3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.