Deprecation from #5930 should not use inspect
jdemeyer opened this issue · 20 comments
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.
CC: @rwst @nbruin @kcrisman @vbraun
Component: misc
Author: Jeroen Demeyer
Branch/Commit: 44e5284
Reviewer: Karl-Dieter Crisman, Ralf Stephan
Issue created by migration from https://trac.sagemath.org/ticket/23159
Description changed:
---
+++
@@ -6,6 +6,6 @@
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=...)")
```
-It's not clear to me what the intent of the condition involving `inspect.ismethod` is...
+It's not clear to me what the intent of the condition involving `inspect.ismethod` is... it was introduced in #2516.
The problem is that this breaks doctests in #22747, which changes some attributes from non-methods to methods.Description changed:
---
+++
@@ -8,4 +8,4 @@
```
It's not clear to me what the intent of the condition involving `inspect.ismethod` is... it was introduced in #2516.
-The problem is that this breaks doctests in #22747, which changes some attributes from non-methods to methods.
+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.Author: Jeroen Demeyer
Description changed:
---
+++
@@ -6,6 +6,6 @@
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=...)")
```
-It's not clear to me what the intent of the condition involving `inspect.ismethod` is... it was introduced in #2516.
+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.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.
I'm ok with the deletion. Using inspect was ad-hoc anyway.
Replying to @kcrisman:
why use
SR.var()instead of justvar()?
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.
Well,
z = var('z')is a bit confusing since it first injectszin the global namespace and then assigns it again to the variablez. So either we change it tosage: 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!
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.
So what should I do?
@kcrisman: 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
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?
Branch pushed to git repo; I updated commit sha1. New commits:
44e5284 | Don't use SR.var in doctests |
That seems fine.
Reviewer: Karl-Dieter Crisman
Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Ralf Stephan
Changed branch from u/jdemeyer/deprecation_from__5930_should_not_use_inspect to 44e5284