#23159 closed defect (fixed)

Deprecation from #5930 should not use inspect

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords:
Cc: rws, nbruin, kcrisman, vbraun Merged in:
Authors: Jeroen Demeyer Reviewers: Karl-Dieter Crisman, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 44e5284 (Commits) Commit: 44e5284f3a4e8ed10bf0c8d908b9d96401ac9af9
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

In src/sage/symbolic/ring.pyx, there is this very unusual code:

            if not hasattr(_the_element,'_fast_callable_') or not inspect.ismethod(_the_element._fast_callable_):
                # only warn if _the_element is not dynamic
                from sage.misc.superseded import deprecation
                deprecation(5930, "Substitution using function-call syntax and unnamed arguments is deprecated and will be removed from a future release of Sage; you can use named arguments instead, like EXPR(x=..., y=...)")

The condition involving inspect.ismethod makes no sense to me... it was introduced in #2516.

The problem is that this breaks doctests in #22747, which changes some attributes from non-methods to methods. The easiest fix is to revert this change from #2516 and unconditionally give the deprecation warning.

Change History (17)

comment:1 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 13 months ago by jdemeyer

  • Branch set to u/jdemeyer/deprecation_from__5930_should_not_use_inspect

comment:4 Changed 13 months ago by jdemeyer

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

New commits:

d288f41Give deprecation warning for #5930 unconditionally

comment:5 Changed 12 months ago by jdemeyer

  • Cc nbruin kcrisman vbraun added
  • Description modified (diff)

comment:6 follow-up: Changed 12 months ago by kcrisman

I don't have a problem with this solution and the doctests seem to be correct. But see here for the exact place rws resolved this (with discussion in the 5-10 comments preceding that one). This comment and this one have further discussion about exact implementation and/or reasons - fast_callable is something I never properly understood, unfortunately.

On a separate note, the cleanups for the hypergeometric plots are nice ... but why use SR.var() instead of just var()? I presume this is to avoid injecting something but I don't want to have to teach people to use SR.var('y') when the current variant is annoying enough.

comment:7 Changed 12 months ago by rws

I'm ok with the deletion. Using inspect was ad-hoc anyway.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 12 months ago by jdemeyer

Replying to kcrisman:

why use SR.var() instead of just var()?

Well, z = var('z') is a bit confusing since it first injects z in the global namespace and then assigns it again to the variable z. So either we change it to

sage: var('z')
z

or

sage: z = SR.var('z')

Anyway, this is minor aspect of the patch which I will gladly revert.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 12 months ago by kcrisman

Well, z = var('z') is a bit confusing since it first injects z in the global namespace and then assigns it again to the variable z. So either we change it to

sage: var('z')
z

This is one of the ways we have had it in the documentation, I think, but I guess somewhere along the line people wanted to not have an output line. But I guess I prefer this to the SR.var since it is more likely to look like something a reader has seen before. Definitely do like the making the hg function and plot in two separate lines, makes them much easier to read!

comment:10 in reply to: ↑ 9 Changed 12 months ago by rws

Replying to kcrisman:

This is one of the ways we have had it in the documentation, I think, but I guess somewhere along the line people wanted to not have an output line.

There is also sporadically _ = var(...) in the documentation.

comment:11 Changed 12 months ago by jdemeyer

So what should I do?

@krisman: do you agree with setting this ticket to positive_review after I change the doctests of the form

sage: z = SR.var('z')

to

sage: var('z')
z

comment:12 Changed 12 months ago by kcrisman

I do not have time to actually run doctests on this but the patchbot says all clear, and it sounds like none of us have a problem with this change, and your doctest change will be helpful in that spirit. So I guess so?

comment:13 Changed 12 months ago by git

  • Commit changed from d288f414bf0f8d785c1928fd1bdce89ec1bdc033 to 44e5284f3a4e8ed10bf0c8d908b9d96401ac9af9

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

44e5284Don't use SR.var in doctests

comment:14 Changed 12 months ago by kcrisman

That seems fine.

comment:15 Changed 12 months ago by jdemeyer

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

comment:16 Changed 12 months ago by kcrisman

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Ralf Stephan

comment:17 Changed 12 months ago by vbraun

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