AsymptoticRing: pass log-function more systematically

[dkrenn]

In #22154 (merged in a recent beta), a new log parameter was introduced. This was very specifically done for this parameter and does not allow any easy extensions. The aim of this ticket is to generalize this by using a general locals parameter. (In this sense it can be seen as a fixup for #22154.)

(Note that no deprecation for log is needed, as this was only merged in a recent beta and not yet in a stable release.)

CC: @behackl

Component: asymptotic expansions

Author: Daniel Krenn

Branch/Commit: 551b052

Reviewer: Benjamin Hackl

Issue created by migration from https://trac.sagemath.org/ticket/27883

[dkrenn]

Branch: u/dkrenn/asy-locals-for-log

[dkrenn]

Last 10 new commits:

627ba1c	Trac #27837: update doctests
26e84eb	Trac #27837: fixup docstrings
687fee5	Trac #27837: fix all doctests in sage.rings.asymptotic
4ac605e	Trac #27837: make variable names more consistent
9aad19c	Trac #27837: fix some more doctests
b4ee24d	Trac #27837: fixup doctest in sage.rings.big_oh
3cb6a45	Trac #27883: class Locals
e4dfe01	Trac #27883: class extension WithLocals
33c951d	Trac #27883: introduce new locals parameter
41a9a65	Trac #27883: use log also in generators

[dkrenn]

Commit: 41a9a65

[dkrenn]

Changed dependencies from #22154 to #22154, #27837

[dkrenn]

comment:4

Trivial dependency #27837 added.

[fchapoton]

comment:5

You are using incompatible-with-python3 syntax, see patchbot report. As said on sage-devel, developers should rather switch now to python3-sage.

[dkrenn]

comment:6

Replying to @fchapoton:

You are using incompatible-with-python3 syntax, see patchbot report.

Without the parenthesis it doesn't work either

>>> di={1:2, 3:4}
>>> sorted(di.items(), key=lambda k,v: k)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: <lambda>() missing 1 required positional argument: 'v'

Not sure, how to nicely rewrite this (I am now using lambda e: e[0] which I find less nice but it works.)

[sagetrac-git]

Changed commit from 41a9a65 to fb8bcf5

[sagetrac-git]

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

fb8bcf5

Trac #27883: fix py3 (lambda)

[dkrenn]

comment:8

Replying to @fchapoton:

You are using incompatible-with-python3 syntax, see patchbot report. As said on sage-devel, developers should rather switch now to python3-sage.

Thank you for telling; I wasn't aware that this is not Py3. (FWIW, I have a Py3-Sage, but I see this ticket more like a fixup of a recent change, so still using my "old" Sage for this. But I will switch soon. ;) )

[sagetrac-git]

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

9be0b63

Trac #27883: remove unneccessary import (pyflakes)

[sagetrac-git]

Changed commit from fb8bcf5 to 9be0b63

[dkrenn]

comment:10

Replying to @dkrenn:

Replying to @fchapoton:

You are using incompatible-with-python3 syntax, see patchbot report.

Without the parenthesis it doesn't work either

>>> di={1:2, 3:4}
>>> sorted(di.items(), key=lambda k,v: k)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: <lambda>() missing 1 required positional argument: 'v'

Not sure, how to nicely rewrite this (I am now using lambda e: e[0] which I find less nice but it works.)

Ok, seems to be just this way according to
https://www.python.org/dev/peps/pep-3113/
They say I should rather use lambda k_v: k_v[0].

[sagetrac-git]

Changed commit from 9be0b63 to 551b052

[sagetrac-git]

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

551b052

Trac #27883: rename variable in lambda-fixup (PEP3113)

[dkrenn]

comment:12

Now patchbots seem to be happy :)

[behackl]

Reviewer: Benjamin Hackl

[behackl]

comment:13

I reviewed the changes and overall, I like this approach to make the module more flexible.

Personally, I would prefer if the log function in the AsymptoticRing still had a parameter to which an alternative log could be passed directly, instead of passing it inside a locals-dictionary; just as a convenience method. What do you think about that?

[dkrenn]

comment:14

Replying to @behackl:

Personally, I would prefer if the log function in the AsymptoticRing still had a parameter to which an alternative log could be passed directly, instead of passing it inside a locals-dictionary; just as a convenience method. What do you think about that?

I thought exactly about doing this. However, I decided not not use it, so that all these kind of parameter passing because:

• When usually something is defined in SageMath globally, it should be overridden in a well defined manner and that's locals.
• This scales to other (future) extensions.
• By using locals (only), it is somehow self explanatory (when knowing what locals is), what it does, whereas I find a log parameter less obvious (although not too bad either).

Therefore, I removed the log parameter completely.

[behackl]

comment:15

Replying to @dkrenn:

Replying to @behackl:

Personally, I would prefer if the log function in the AsymptoticRing still had a parameter to which an alternative log could be passed directly, instead of passing it inside a locals-dictionary; just as a convenience method. What do you think about that?

I thought exactly about doing this. However, I decided not not use it, so that all these kind of parameter passing because:

• When usually something is defined in SageMath globally, it should be overridden in a well defined manner and that's locals.
• This scales to other (future) extensions.
• By using locals (only), it is somehow self explanatory (when knowing what locals is), what it does, whereas I find a log parameter less obvious (although not too bad either).

Therefore, I removed the log parameter completely.

Alright. I see, and agree with your conclusion. (Besides, I would assume that if someone wants to use a custom log function, then they should set it globally for their entire AsymptoticRing anyways; adapting the log locally probably isn't really that common anyways.)

I want to do some further investigations regarding this ticket; I should be done soon.

[behackl]

comment:16

Of course, I forgot to set this to positive_review after I was done.

I reviewed the changes carefully, everything looks good to me and also the patchbot seems to be happy.

[fchapoton]

Changed dependencies from #22154, #27837 to none

[dkrenn]

comment:18

Replying to @fchapoton:

Dependencies #22154, #27837 deleted

I am curious: Why were the (closed) dependencies deleted? Is there any advantage for doing this and should we do this on a regular basis? (However, I do not mind that it was done, just curious as said.) ;)

[fchapoton]

comment:19

This may be blocking the merge by the release manager. Not sure, but just to be safe.

[dkrenn]

comment:20

Replying to @fchapoton:

This may be blocking the merge by the release manager. Not sure, but just to be safe.

I see; I think this is not an issue here, see https://groups.google.com/d/msg/sage-release/E4vp20O2FN0/Sk5IynzSAwAJ (it will be in rc1).

[vbraun]

Changed branch from u/dkrenn/asy-locals-for-log to 551b052