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:  sage5.0 
Component:  misc  Keywords:  deprecation 
Cc:  Florent Hivert  Merged in:  sage5.0.beta10 
Authors:  Luca De Feo, Florent Hivert  Reviewers:  Rob Beezer 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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:
Attachments (4)
Change History (27)
Changed 11 years ago by
Attachment:  trac_11585_depracted_function_alias.patch added 

comment:1 Changed 11 years ago by
Status:  new → needs_review 

comment:2 Changed 11 years ago by
Cc:  Florent Hivert added 

Description:  modified (diff) 
Reviewers:  → Rob Beezer 
Status:  needs_review → needs_work 
Changed 11 years ago by
Attachment:  trac_11585_deprecated_function_alias.patch added 

comment:3 Changed 11 years ago by
Description:  modified (diff) 

Status:  needs_work → needs_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
Status:  needs_review → positive_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
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
Status:  positive_review → needs_work 

This needs to be rebased to sage4.7.1.rc1
Changed 11 years ago by
Attachment:  trac_11585_deprecated_function_alias+rc1.patch added 

Rebased against rc1
comment:7 followup: 8 Changed 11 years ago by
Description:  modified (diff) 

Status:  needs_work → needs_review 
comment:8 Changed 11 years ago by
Reviewers:  Rob Beezer → Rob Beezer, Florent Hivert 

Status:  needs_review → positive_review 
If you just rebased the patch, you are allowed to put a positive review yourself. Anyway, I made a short rereview and found it Ok.
Florent
comment:9 Changed 11 years ago by
Merged in:  → sage4.7.2.alpha1 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:10 Changed 11 years ago by
Merged in:  sage4.7.2.alpha1 

Resolution:  fixed 
Status:  closed → new 
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
Status:  new → needs_review 

comment:12 Changed 11 years ago by
Status:  needs_review → needs_work 

Work issues:  → startuptime 
comment:13 followup: 14 Changed 11 years ago by
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 Changed 11 years ago by
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
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
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
Authors:  Luca De Feo → Luca De Feo, Florent Hivert 

Reviewers:  Rob Beezer, Florent Hivert → Rob 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 2387 2387  Florent Hivert (20091123), with the help of Mike Hansen. 2388 2388  Luca De Feo (20110711), printing the full module path when different from old path 2389 2389 """ 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: 2392 2393 module_name = '__main__' 2393 else:2394 module_name = module.__name__2395 2394 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
Attachment:  trac_11585_deprecated_function_aliasrc2.patch added 

comment:18 Changed 11 years ago by
Description:  modified (diff) 

Status:  needs_work → needs_review 
comment:19 Changed 11 years ago by
Owner:  set to Florent Hivert 

comment:20 followup: 21 Changed 11 years ago by
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 Changed 11 years ago by
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
Status:  needs_review → positive_review 

comment:23 Changed 11 years ago by
Merged in:  → sage5.0.beta10 

Resolution:  → fixed 
Status:  positive_review → closed 
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 doublecolon 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:
should be formatted as
See http://www.sagemath.org/doc/developer/conventions.html
which references: http://www.python.org/dev/peps/pep0008/
(look shortly after "Compound statements")