Opened 2 years ago

Closed 2 years ago

#23162 closed enhancement (fixed)

Give a warning when using citation.get_systems() without Cython profiling

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: cython Keywords: thursdaysbdx
Cc: slabbe Merged in:
Authors: Jeroen Demeyer Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: ae120c5 (Commits) Commit: ae120c54d415c6cad3b7fb3f25d8988e62abc441
Dependencies: Stopgaps:

Description

sage.misc.citation.get_systems() is implemented by using cProfile to look at which modules implement the functions called when executing the code.

The problem is that this is totally unreliable when Cython is compiled without profiling support (which is the default). This doctest

sage: from sage.misc.citation import get_systems
sage: get_systems('((x+1)^2).expand()')
['ginac']

only works because Expression.expand() is called by Python instead of Cython. If that call would be inside some other Cython code, then Python's profiler would not detect it:

sage: cython('def callexpand(x): return x.expand()')
sage: from sage.misc.citation import get_systems
sage: get_systems('callexpand(((x+1)^2))')
[]

There is a problem because #22747 will "break" profiling even further as even the top-level call of Expression.expand() would not be detected as something to be entered in the profiler.

So here I propose simply to give a warning whenever get_systems() is used when profiling was not enabled.

Change History (6)

comment:1 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/give_a_warning_when_using_citation_get_systems___without_cython_profiling

comment:2 Changed 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to ae120c54d415c6cad3b7fb3f25d8988e62abc441
  • Status changed from new to needs_review

New commits:

ae120c5Give a warning when using citation.get_systems() without Cython profiling

comment:3 Changed 2 years ago by jdemeyer

  • Cc slabbe added

comment:4 Changed 2 years ago by slabbe

  • Reviewers set to Sébastien Labbé
  • Status changed from needs_review to positive_review

Works for me on OSX 10.10. sage -t --long works. Code looks good. Documentation builds and looks fine.

comment:5 Changed 2 years ago by slabbe

  • Keywords thursdaysbdx added

comment:6 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/give_a_warning_when_using_citation_get_systems___without_cython_profiling to ae120c54d415c6cad3b7fb3f25d8988e62abc441
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.