Opened 11 years ago

Closed 11 years ago

#11585 closed enhancement (fixed)

Make deprecated_function_alias print the whole module path when it differs from the original

Reported by: Luca De Feo Owned by: Florent Hivert
Priority: minor Milestone: sage-5.0
Component: misc Keywords: deprecation
Cc: Florent Hivert Merged in: sage-5.0.beta10
Authors: Luca De Feo, Florent Hivert Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Florent Hivert)

Currently

sage: from sage.misc.misc import deprecated_function_alias
sage: a = deprecated_function_alias(sqrt, "Version ?")
sage: a(5)
...
DeprecationWarning: (Since Version ?) a is deprecated. Please use sqrt instead.
sqrt(5)

which is misleading because the user would expect to find sqrt in the same module as a.

With this patch

sage: from sage.misc.misc import deprecated_function_alias
sage: a = deprecated_function_alias(sqrt, "Version ?")
sage: a(5)
...
DeprecationWarning: (Since Version ?) a is deprecated. Please use sage.functions.other.sqrt instead.
sqrt(5)

There's no change in deprecating methods.

Apply:

  1. trac_11585_deprecated_function_alias-rc2.patch

Attachments (4)

trac_11585_depracted_function_alias.patch (3.5 KB) - added by Luca De Feo 11 years ago.
trac_11585_deprecated_function_alias.patch (3.5 KB) - added by Luca De Feo 11 years ago.
trac_11585_deprecated_function_alias+rc1.patch (3.6 KB) - added by Luca De Feo 11 years ago.
Rebased against rc1
trac_11585_deprecated_function_alias-rc2.patch (3.7 KB) - added by Florent Hivert 11 years ago.

Download all attachments as: .zip

Change History (27)

Changed 11 years ago by Luca De Feo

comment:1 Changed 11 years ago by Luca De Feo

Status: newneeds_review

comment:2 Changed 11 years ago by Rob Beezer

Cc: Florent Hivert added
Description: modified (diff)
Reviewers: Rob Beezer
Status: needs_reviewneeds_work

Looks good. Passes long tests on 4.7.1.alpha3. I've cc'ed Florent Hivert in case he wants to add anything. I've also added an "Apply" section to the description for the release manager.

Two minor formatting items need attention.

Line 2287: Trac #11585::

You need a blank line after the double-colon to make the verbatim text format properly.

You can test documentation via sage -docbuild reference html after a fresh build (sage -b) and then viewing the resulting HTML file.

Line 2299:

if module is None: module_name = '__main__'
else: module_name = module.__name__

should be formatted as

if module is None:
    module_name = '__main__'
else:
    module_name = module.__name__

See http://www.sagemath.org/doc/developer/conventions.html

which references: http://www.python.org/dev/peps/pep-0008/

(look shortly after "Compound statements")

Changed 11 years ago by Luca De Feo

comment:3 Changed 11 years ago by Luca De Feo

Description: modified (diff)
Status: needs_workneeds_review

Modified as requested by rbeezer. Apply second patch only (sorry for the mispelling in the first patch).

comment:4 Changed 11 years ago by Rob Beezer

Status: needs_reviewpositive_review

Luca,

Looks good - thanks for the improvement.

I am going to switch this to positive review, but Florent Hivert should feel free to get in on this just in case he sees something.

Rob

comment:5 Changed 11 years ago by Florent Hivert

Description: modified (diff)

I've no particular comments concerning this patch. We should start to actually remove deprecated things from sage, but this is another problem. Ready to go.

Florent

comment:6 Changed 11 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

This needs to be rebased to sage-4.7.1.rc1

Changed 11 years ago by Luca De Feo

Rebased against rc1

comment:7 Changed 11 years ago by Luca De Feo

Description: modified (diff)
Status: needs_workneeds_review

comment:8 in reply to:  7 Changed 11 years ago by Florent Hivert

Reviewers: Rob BeezerRob Beezer, Florent Hivert
Status: needs_reviewpositive_review

If you just rebased the patch, you are allowed to put a positive review yourself. Anyway, I made a short re-review and found it Ok.

Florent

comment:9 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.7.2.alpha1
Resolution: fixed
Status: positive_reviewclosed

comment:10 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.7.2.alpha1
Resolution: fixed
Status: closednew

This patch greatly increases the Sage startup time. On sage.math.washington.edu, before the patch it is roughly 1.4s and after the patch 2.2s.

comment:11 Changed 11 years ago by Jeroen Demeyer

Status: newneeds_review

comment:12 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work
Work issues: startuptime

comment:13 Changed 11 years ago by Luca De Feo

Ouch! This single line is responsible for it:

module = inspect.getmodule(inspect.stack()[1][0])

In fact, there's about 40 calls to deprecated_function_alias in 4.7.1.rc2; on my laptop:

sage: import inspect
sage: timeit('inspect.stack()[1][0]')
25 loops, best of 3: 18.4 ms per loop

The fact is, I don't know how to obtain the name of the module where the call to deprecated_function_alias happened without inspecting the stack. Any ideas?

comment:14 in reply to:  13 Changed 11 years ago by Florent Hivert

The fact is, I don't know how to obtain the name of the module where the call to deprecated_function_alias happened without inspecting the stack. Any ideas?

I don't have to investigate further now but I think you don't need the full stack but only the top of it. Using

inspect.getframeinfo(sys._getframe(1),1)

should be much faster...

comment:15 Changed 11 years ago by Florent Hivert

Owner: Jason Grout deleted

Here is what I think is a good solution inspect.currentframe(1) does the same as inspect.stack()[1][0] only 400 time faster:

sage: import inspect
sage: inspect.getmodule(inspect.stack()[1][0]) == inspect.getmodule(inspect.currentframe(1))
True
sage: timeit('inspect.getmodule(inspect.stack()[1][0])')
25 loops, best of 3: 15.6 ms per loop
sage: timeit('inspect.getmodule(inspect.currentframe(1))')
625 loops, best of 3: 40.7 µs per loop

I'm updating the patch accordingly.

Florent

comment:16 Changed 11 years ago by Florent Hivert

Unfortunately, the problem is not with the stack:

Before any patch:

sage.all: 0.987 (None)

After the rc1 patch

sage.all: 1.805 (None)

After the rc2 patch

sage.all: 1.692 (None)

It's better but still too much.

Florent

comment:17 Changed 11 years ago by Florent Hivert

Authors: Luca De FeoLuca De Feo, Florent Hivert
Reviewers: Rob Beezer, Florent HivertRob Beezer

Got it !!! We don't need the module but only its name. One can get it using the filename of the code which is currently executed. Here is the diff

  • sage/misc/misc.py

    diff --git a/sage/misc/misc.py b/sage/misc/misc.py
    a b def deprecated_function_alias(func, vers 
    23872387     - Florent Hivert (2009-11-23), with the help of Mike Hansen.
    23882388     - Luca De Feo (2011-07-11), printing the full module path when different from old path
    23892389    """
    2390     module = inspect.getmodule(inspect.currentframe(1))
    2391     if module is None:
     2390    module_name = inspect.getmodulename(
     2391        inspect.currentframe(1).f_code.co_filename)
     2392    if module_name is None:
    23922393        module_name = '__main__'
    2393     else:
    2394         module_name = module.__name__
    23952394    return DeprecatedFunctionAlias(func, version, module_name)

The new startup time is ok !

sage.all: 0.962 (None)

Please review.

Florent

Changed 11 years ago by Florent Hivert

comment:18 Changed 11 years ago by Florent Hivert

Description: modified (diff)
Status: needs_workneeds_review

comment:19 Changed 11 years ago by Florent Hivert

Owner: set to Florent Hivert

comment:20 Changed 11 years ago by Luca De Feo

Thanks for sorting this out, Florent.

I tried it: it solves the startup issue and all tests pass. Can I give positive review, or shall we ask someone else to review it?

Luca

comment:21 in reply to:  20 Changed 11 years ago by Florent Hivert

Work issues: startuptime

I tried it: it solves the startup issue and all tests pass. Can I give positive review, or shall we ask someone else to review it?

Sure you can. Your code has already been reviewed twice so it is Ok to go. Given that, only my change has to be reviewed, it could be done by person which is not me. I think you are the perfect person to do that. So please set the positive review if you think my change is Ok.

comment:22 Changed 11 years ago by Luca De Feo

Status: needs_reviewpositive_review

comment:23 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta10
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.