Opened 10 years ago
Closed 15 months ago
#14270 closed defect (fixed)
Remove function call syntax for symbolic expressions
Reported by:  Punarbasu Purkayastha  Owned by:  Burcin Erocal 

Priority:  major  Milestone:  sage9.5 
Component:  symbolics  Keywords:  
Cc:  Nicolas M. Thiéry, Keshav Kini, Dave Morris, Nils Bruin, Eric Gourgoulhon  Merged in:  
Authors:  Punarbasu Purkayastha, Matthias Koeppe  Reviewers:  Matthias Koeppe, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  9891a51 (Commits, GitHub, GitLab)  Commit:  9891a512409a4c03a3abc42a51f935ac001d58e6 
Dependencies:  Stopgaps: 
Description (last modified by )
The function call syntax for symbolic expressions has been deprecated for about 12 years. It is about time it raised some errors. This will prevent people from getting confused because of behavior like this.
See also the threads
 http://groups.google.com/group/sagedevel/t/de97f91d548cc0ec
 http://groups.google.com/group/sagedevel/t/5f2705b2b8d21593/b11079d8299a03b5
Update in 9.4.beta4: The deprecation warning introduced in #5930 was not issued in direct interactive use since Sage 8.4 (despite being doctested):
┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 9.4.beta4, Release Date: 20210701 │ │ Using Python 3.9.5. Type "help()" for help. │ └────────────────────────────────────────────────────────────────────┘ ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ ┃ Warning: this is a prerelease version, and it may be unstable. ┃ ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ sage: var('y') y sage: a = x+y sage: a(2, 3) 5 sage: warnings.resetwarnings() sage: a(2, 3) /Users/mkoeppe/s/sage/sagerebasing/worktreegcc11/local/lib/python3.9/sitepackages/IPython/core/interactiveshell.py:3343: DeprecationWarning: Substitution using functioncall 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=...) See http://trac.sagemath.org/5930 for details. exec(code_obj, self.user_global_ns, self.user_ns) 5
(fixed in #32139)
Followup: #32227 Deprecate methods arguments
(alias: args
), number_of_arguments
, _fast_callable_
for noncallable symbolic expressions
Attachments (1)
Change History (55)
comment:1 Changed 10 years ago by
Authors:  → Punarbasu Purkayastha 

Description:  modified (diff) 
Status:  new → needs_review 
comment:2 Changed 10 years ago by
Status:  needs_review → needs_work 

Work issues:  → fix doctests 
comment:3 followups: 4 14 Changed 10 years ago by
Work issues:  fix doctests → fix combinat/tutorial.py 

Changed 10 years ago by
Attachment:  trac_14270raise_error_on_functioncall.patch added 

Apply to devel/sage
comment:4 Changed 10 years ago by
Cc:  Nicolas M. Thiéry added 

Replying to ppurka:
I am able to fix all the doctests except for the following one in combinat/tutorial.py.
<snip>
What should I do with this portion of the tutorial? Delete this?
I'm cc:ing Nicolas, who should know what is going on here.
comment:5 followups: 6 16 Changed 10 years ago by
I'd also say that I think the prep tutorial one should still talk about this at some length, to explain (in the event this is done) why this doesn't work, because a lot of people will now and forevermore expect that it will work. Similarly, most of these examples presumably should be moved to the new syntax, perhaps even with an explanatory remark as to exactly why that is the syntax. For instance, how would one do the matrix example h(x)
now? We should be careful not to remove anything, just to change how it works to the appropriate way postthisticket.
Rant I don't actually want to rehash any more, but put here for completeness:
<rant> Because distinguishing between a function and a symbolic expression is an unnatural, CSy thing to do; any symbolic expression is obviously a function of all variables in it, mathematically; for any function with more than one
var
I agree we don't want this (as indeed the example in the prep document points out) but for onevariable expressions (pace Jason, who will immediately ask whetherx+yy
is one variable) it seems worth the potential for confusion... </rant>
comment:6 Changed 10 years ago by
Replying to kcrisman:
I'd also say that I think the prep tutorial one should still talk about this at some length, to explain (in the event this is done) why this doesn't work, because a lot of people will now and forevermore expect that it will work.
IMHO, the people who expect this to still work need to change their code. It has been in deprecated mode for over four years. That's more than enough time to change their habit and old code. I think someone hasn't complained before either because they are complacent or because they don't use this at all.
While teaching a course with Sage, I remember that we ourselves ran into this problem with the students. It was annoying and confusing because we were unaware of the code and how to fix it. We would just ask the students to ignore those warnings. What would a beginner do after defining f(x) = x^2
? The most natural thing would be to do f(2)
or something similar to "see" that it can actually evaluate. Now imagine the same with f = x^2
and what you get is the deprecation message and then the correct answer. Second time it is evaluated, there is no deprecation message, so a beginner will wonder what just happened in the first invocation. It is not a favorable impression. It gives the impression of a halfdone software.
I agree with you that it should be explained in the tutorial that there is a difference between symbolic functions and symbolic expressions and python functions.
comment:7 Changed 10 years ago by
Cc:  Keshav Kini added 

comment:8 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:9 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:10 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:11 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:12 Changed 8 years ago by
Description:  modified (diff) 

This is actually the same as #8214, but that has no code, so I'm copying the two links from there to here before declaring it a dupe.
comment:13 Changed 8 years ago by
This ticket is unlikely to get fixed unless someone who knows the code really well addresses comment:3
comment:14 Changed 8 years ago by
Replying to ppurka:
I am able to fix all the doctests except for the following one in combinat/tutorial.py.
OK, from what you've written down here, I can come up with one way of rewriting the example.
sage: C, z = var('C,z'); sage: sys = [ C == z + C*C ] sage: sol = solve(sys, C, solution_dict=True); sol [{C: 1/2*sqrt(4*z + 1) + 1/2}, {C: 1/2*sqrt(4*z + 1) + 1/2}] sage: s0 = sol[0][C]; s1 = sol[1][C]
sage: C = s0; C 1/2*sqrt(4*z + 1) + 1/2
I think this should be deleted. Use s0 if you want that and just leave C bound to the variable. It's already bad enough that we need "C as a function" further down.
sage: equadiff (4*z  1)*D[0](C)(z)  2*C(z) + 1 == 0 sage: Cf = sage.symbolic.function_factory.function('C')
Didn't we need Cf before to arrive at equadiff
? How did C(z)
get into that expression in the first place?
sage: equadiff.substitute_function(Cf, s0) # Original answer is the deprecation + answer ... TypeError: %d format: a number is required, not NoneType
I think the following should work (it does in vanilla, without a deprecation warning):
sage: equadiff.substitute_function(Cf, s0.function(z))
Let us try to "fix" this (
z
is a symbolic variable). Somehow the following works (beats me why it does):sage: equadiff.substitute_function(Cf(z), s0) (4*z  1)*D[0](C)(z)  2*C(z) + 1 == 0 # OK, so this seems to work
I'm not so sure it "works". It doesn't give an error (which is surprising in its own right; perhaps that should change), but I don't think it gives the same answer. In vanilla:
sage: equadiff.substitute_function(Cf(z), s0) (4*z  1)*D[0](C)(z)  2*C(z) + 1 == 0 sage: equadiff.substitute_function(Cf, s0.function(z)) (4*z  1)/sqrt(4*z + 1) + sqrt(4*z + 1) == 0
But now the next command in the tutorial gives
False
instead ofTrue
Rightly so, judging from the results above.
Why is all this happening? Looking at the documentation of
substitute_function
shows that it should be used for substituting functions, not anything elsedef substitute_function(self, original, new): """ Returns this symbolic expressions all occurrences of the function *original* replaced with the function *new*.And what exactly are we substituting above?
sage: type(Cf) sage.symbolic.function_factory.NewSymbolicFunction sage: type(s0) sage.symbolic.expression.ExpressionWe are substituting a symbolic function with a symbolic expression! How was this even working earlier?
I guess because symbolic expressions were deprecated "functions": you could call then with symbolic arguments and get a symbolic expression, at the cost of a deprecation warning.
What should I do with this portion of the tutorial? Delete this?
No, just do
sage: equadiff.substitute_function(Cf, s0.function(z))
That's probably a useful example to have anyway. It shows how to turn a symbolic expression into a function in a nonambiguous way, because you need to specify argument order.
comment:15 Changed 8 years ago by
Description:  modified (diff) 

comment:16 followups: 18 21 Changed 5 years ago by
Replying to kcrisman:
<rant> Because distinguishing between a function and a symbolic expression is an unnatural, CSy thing to do; any symbolic expression is obviously a function of all variables in it, mathematically; for any function with more than one
var
I agree we don't want this (as indeed the example in the prep document points out) but for onevariable expressions (pace Jason, who will immediately ask whetherx+yy
is one variable) it seems worth the potential for confusion... </rant>
I actually agree and I don't see why a function call with nonrelational argument should not be allowed IF there is only one variable in the expression. The confusion comes from the fact that things like sqrt(2)(x+y)
almost always are typos where the *
is missing. These should be marked as error. Now Sage cannot distinguish between f=x^2; f(2)
and sqrt(2)(x+y)
because f
is replaced immediately with x^2
and so in both cases an expression has its __call__
member executed. BUT, first with sqrt(2)(x+y)
the expression has no variables and so it can be doubtlessy marked as error; secondly, before f
is silently expanded to x^2
it is seen by the parsermaybe the parser can distinguish between calls that are done to f
(like calls to functions) and other calls, and only allow the former?
comment:17 Changed 5 years ago by
Milestone:  sage6.4 → sage8.2 

Status:  needs_work → needs_info 
comment:18 Changed 5 years ago by
Replying to rws:
maybe the parser can distinguish between calls that are done to
f
(like calls to functions) and other calls, and only allow the former?
That would be adding even more special cases just for Sage.
In Python, you expect the following two to do the same thing
f = x^2 print(f(2))
and
print((x^2)(2))
It would be very surprising if somehow these would become different.
comment:19 Changed 5 years ago by
But of course Jason's x+yy
still has to be dealt with, if only to ignore it ...
comment:20 Changed 5 years ago by
This is equality (or zero) proving in the symbolic ring, so it can be arbitrarily complex. Ignore it, yes.
comment:21 Changed 5 years ago by
... The confusion comes from the fact that things like
sqrt(2)(x+y)
almost always are typos where the*
is missing. These should be marked as error.
This is now #23684
comment:22 Changed 17 months ago by
Milestone:  sage8.2 → sage9.4 

Status:  needs_info → needs_work 
comment:23 Changed 17 months ago by
Cc:  Dave Morris added 

comment:24 Changed 17 months ago by
Description:  modified (diff) 

comment:25 Changed 17 months ago by
Dependencies:  → #32139 

comment:26 Changed 17 months ago by
Description:  modified (diff) 

comment:27 Changed 17 months ago by
Branch:  → u/mkoeppe/remove_function_call_syntax_for_symbolic_expressions 

comment:28 Changed 17 months ago by
Authors:  Punarbasu Purkayastha → Punarbasu Purkayastha, Matthias Koeppe 

Commit:  → 1735601f1a8221ec9b97eac3ec2be11797818318 
New commits:
4191337  src/sage/all.py: Repair the filterwarnings regexp that ensures sagelib warnings are not ignored

6492b8c  src/sage/symbolic/ring.pyx: Pass the correct stacklevel to deprecatio()

04fdda6  sage.all: In filterwarnings, remove leading '.*' from module regexes, use regex that matches vendored 'packaging'

a6691b4  Merge #32139

9f5c197  remove function call syntax from symbolic expressions

1735601  SymbolicRing: Update documentation and tests

comment:29 Changed 17 months ago by
Cc:  Nils Bruin Eric Gourgoulhon added 

Status:  needs_work → needs_review 
Work issues:  fix combinat/tutorial.py 
comment:30 Changed 17 months ago by
Commit:  1735601f1a8221ec9b97eac3ec2be11797818318 → fb48800b124fdfb1c9c43de368e687f0df973411 

comment:31 Changed 17 months ago by
Description:  modified (diff) 

comment:32 Changed 17 months ago by
Status:  needs_review → needs_work 

patchbot found some doctest failures
comment:33 Changed 17 months ago by
Commit:  fb48800b124fdfb1c9c43de368e687f0df973411 → 784782c27b42b9ebcf5126c93a61449fe36a477c 

Branch pushed to git repo; I updated commit sha1. New commits:
98f7af1  src/sage/tests/books/computationalmathematicswithsagemath/integration_doctest.py: Avoid deprecated use of an expression as a callable

784782c  Expression: Deprecate arguments, args, number_of_arguments for noncallable expressions

comment:34 Changed 17 months ago by
Description:  modified (diff) 

comment:35 Changed 17 months ago by
Description:  modified (diff) 

comment:36 Changed 17 months ago by
Commit:  784782c27b42b9ebcf5126c93a61449fe36a477c → 6566e8d64d1ad3059e5656e2ddd095f24515512b 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6566e8d  src/sage/combinat/tutorial.py: Remove use of expression call

comment:37 Changed 17 months ago by
Description:  modified (diff) 

Status:  needs_work → needs_review 
comment:38 Changed 17 months ago by
Status:  needs_review → needs_work 

comment:39 Changed 17 months ago by
Commit:  6566e8d64d1ad3059e5656e2ddd095f24515512b → b12d1b022899b690b55e2e9f36d06e73ef2f1fa8 

Branch pushed to git repo; I updated commit sha1. New commits:
b12d1b0  src/sage/symbolic/expression.pyx: Remove doctest of removed feature

comment:41 Changed 17 months ago by
Dependencies:  #32139 → #32139, #32233 

comment:42 Changed 17 months ago by
Commit:  b12d1b022899b690b55e2e9f36d06e73ef2f1fa8 → 4063e39a16fc4a5f63060af5f339b0d73c1791db 

Branch pushed to git repo; I updated commit sha1. New commits:
94d7ce4  PrimitiveFunction: Remove

945e802  SFunction: Remove

905a234  DeprecatedSFunction: Remove

48dff5b  deprecated_custom_evalf_wrapper: Remove

8167cb6  Merge #32233

4063e39  fast_callable: Convert expressions to callable symbolic expressions

comment:43 Changed 17 months ago by
Status:  needs_work → needs_review 

As fast_callable
is used in plotting, this should be checked carefully for any regressions
comment:44 Changed 16 months ago by
Status:  needs_review → needs_work 

sage t long randomseed=0 src/sage/calculus/desolvers.py ********************************************************************** File "src/sage/calculus/desolvers.py", line 1609, in sage.calculus.desolvers.? Failed example: sol=desolve_odeint(f,[0.5,2],srange(0,10,0.1),[x,y]) Exception raised: Traceback (most recent call last): File "/home/sagepatchbot/sage/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 718, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/sagepatchbot/sage/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 1137, in compile_and_execute exec(compiled, globs) File "<doctest sage.calculus.desolvers.?[3]>", line 1, in <module> sol=desolve_odeint(f,[RealNumber('0.5'),Integer(2)],srange(Integer(0),Integer(10),RealNumber('0.1')),[x,y]) File "/home/sagepatchbot/sage/local/lib/python3.8/sitepackages/sage/calculus/desolvers.py", line 1726, in desolve_odeint return desolve_odeint_inner(ivar) File "/home/sagepatchbot/sage/local/lib/python3.8/sitepackages/sage/calculus/desolvers.py", line 1679, in desolve_odeint_inner desc.append(fast_float(de,*variabs)) File "sage/ext/fast_eval.pyx", line 1391, in sage.ext.fast_eval.fast_float (build/cythonized/sage/ext/fast_eval.c:11249) return fast_callable(f, vars=vars, domain=float, File "sage/ext/fast_callable.pyx", line 457, in sage.ext.fast_callable.fast_callable (build/cythonized/sage/ext/fast_callable.c:4734) vars = [to_var(var) for var in vars] File "sage/ext/fast_callable.pyx", line 456, in sage.ext.fast_callable.fast_callable.to_var (build/cythonized/sage/ext/fast_callable.c:4147) return SR.var(var) File "sage/symbolic/ring.pyx", line 999, in sage.symbolic.ring.SymbolicRing.var (build/cythonized/sage/symbolic/ring.cpp:11232) raise ValueError(f'The name "{s}" is not a valid Python identifier.') ValueError: The name "(symbol1790" is not a valid Python identifier.
comment:45 Changed 16 months ago by
Dependencies:  #32139, #32233 → #32139, #32233, #29738 

comment:46 Changed 16 months ago by
Commit:  4063e39a16fc4a5f63060af5f339b0d73c1791db → e198c5dcf743e97414d4de19e63770c059e9724c 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ae0a28e  Expression.__len__: Deprecate

65881e9  Expression.__len__: Update doctest output

5690782  ParametrizedSurface3D.__init__: Do not call len on something that could be either a variable or a list/tuple

3575a8f  sage.calculus.desolvers.desolve_odeint: Simplify code, avoid calling __len__ on symbolic expressions

75d7751  Merge #32139

4a63e61  Trac #29738: update/fix doctest in asymptotics_multivariate_generating_functions.py

e40f9fe  Merge tag '9.4.beta5' into t/29738/perhaps_symbolic_expressions_should_not_have_a_length

75f6533  desolve_odeint: Simplify code

86b0108  Expression.number_of_operands, __len__: Update docstrings to use the word 'operands' instead of 'arguments'

e198c5d  Merge #29738

comment:47 Changed 16 months ago by
Status:  needs_work → needs_review 

comment:48 Changed 16 months ago by
Status:  needs_review → needs_work 

I think this needs a rebase onto rc0? Trac merge failed even though it rebases cleanly.
comment:49 Changed 16 months ago by
Commit:  e198c5dcf743e97414d4de19e63770c059e9724c → 9891a512409a4c03a3abc42a51f935ac001d58e6 

Branch pushed to git repo; I updated commit sha1. New commits:
9891a51  Merge tag '9.4.rc0' into t/14270/remove_function_call_syntax_for_symbolic_expressions

comment:50 Changed 16 months ago by
Status:  needs_work → needs_review 

comment:51 Changed 16 months ago by
Dependencies:  #32139, #32233, #29738 

Reviewers:  → Matthias Koeppe, ... 
comment:52 Changed 16 months ago by
Milestone:  sage9.4 → sage9.5 

Reviewers:  Matthias Koeppe, ... → Matthias Koeppe, Travis Scrimshaw 
Status:  needs_review → positive_review 
We have given people plenty of time to stop using this. Time to bring down the hammer. I am setting this to a positive review, but setting it to 9.5 just to make sure it does not go into the soontobereleased 9.4 to get more testing.
comment:54 Changed 15 months ago by
Branch:  u/mkoeppe/remove_function_call_syntax_for_symbolic_expressions → 9891a512409a4c03a3abc42a51f935ac001d58e6 

Resolution:  → fixed 
Status:  positive_review → closed 
I am able to fix all the doctests except for the following one in combinat/tutorial.py.
The example which fails seems to make no sense to me. A
substitute_function
method is being used to substitute a function with a symbolic expression.Let us try to "fix" this (
z
is a symbolic variable). Somehow the following works (beats me why it does):But now the next command in the tutorial gives
False
instead ofTrue
Why is all this happening? Looking at the documentation of
substitute_function
shows that it should be used for substituting functions, not anything elseAnd what exactly are we substituting above?
We are substituting a symbolic function with a symbolic expression! How was this even working earlier?
What should I do with this portion of the tutorial? Delete this?