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: sage-9.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:

Status badges

Description (last modified by Matthias Köppe)

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

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: 2021-07-01               │
│ 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/sage-rebasing/worktree-gcc11/local/lib/python3.9/site-packages/IPython/core/interactiveshell.py:3343: DeprecationWarning: 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=...)
See http://trac.sagemath.org/5930 for details.
  exec(code_obj, self.user_global_ns, self.user_ns)
5

(fixed in #32139)

Follow-up: #32227 Deprecate methods arguments (alias: args), number_of_arguments, _fast_callable_ for non-callable symbolic expressions

Attachments (1)

trac_14270-raise_error_on_function-call.patch (14.0 KB) - added by Punarbasu Purkayastha 10 years ago.
Apply to devel/sage

Download all attachments as: .zip

Change History (55)

comment:1 Changed 10 years ago by Punarbasu Purkayastha

Authors: Punarbasu Purkayastha
Description: modified (diff)
Status: newneeds_review

comment:2 Changed 10 years ago by Punarbasu Purkayastha

Status: needs_reviewneeds_work
Work issues: fix doctests

comment:3 Changed 10 years ago by Punarbasu Purkayastha

Work issues: fix doctestsfix combinat/tutorial.py

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.

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

...

sage: equadiff
(4*z - 1)*D[0](C)(z) - 2*C(z) + 1 == 0
sage: Cf = sage.symbolic.function_factory.function('C')
sage: equadiff.substitute_function(Cf, s0)  # Original answer is the deprecation + answer
...
TypeError: %d format: a number is required, not NoneType

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

But now the next command in the tutorial gives False instead of True

sage: bool(equadiff.substitute_function(Cf(z), s0))  
False

Why is all this happening? Looking at the documentation of substitute_function shows that it should be used for substituting functions, not anything else

    def 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.Expression

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?

Changed 10 years ago by Punarbasu Purkayastha

Apply to devel/sage

comment:4 in reply to:  3 Changed 10 years ago by Karl-Dieter Crisman

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 Changed 10 years ago by Karl-Dieter Crisman

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 post-this-ticket.

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, CS-y 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 one-variable expressions (pace Jason, who will immediately ask whether x+y-y is one variable) it seems worth the potential for confusion... </rant>

comment:6 in reply to:  5 Changed 10 years ago by Punarbasu Purkayastha

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 half-done 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 Keshav Kini

Cc: Keshav Kini added

comment:8 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:9 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:10 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:11 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:12 Changed 8 years ago by Ralf Stephan

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 Punarbasu Purkayastha

This ticket is unlikely to get fixed unless someone who knows the code really well addresses comment:3

comment:14 in reply to:  3 Changed 8 years ago by Nils Bruin

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 of True

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 else

    def 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.Expression

We 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 non-ambiguous way, because you need to specify argument order.

Last edited 8 years ago by Nils Bruin (previous) (diff)

comment:15 Changed 8 years ago by Keshav Kini

Description: modified (diff)

comment:16 in reply to:  5 ; Changed 5 years ago by Ralf Stephan

Replying to kcrisman:

<rant> Because distinguishing between a function and a symbolic expression is an unnatural, CS-y 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 one-variable expressions (pace Jason, who will immediately ask whether x+y-y is one variable) it seems worth the potential for confusion... </rant>

I actually agree and I don't see why a function call with non-relational 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 parser---maybe 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 Ralf Stephan

Milestone: sage-6.4sage-8.2
Status: needs_workneeds_info

comment:18 in reply to:  16 Changed 5 years ago by Jeroen Demeyer

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 Karl-Dieter Crisman

But of course Jason's x+y-y still has to be dealt with, if only to ignore it ...

comment:20 Changed 5 years ago by Ralf Stephan

This is equality (or zero) proving in the symbolic ring, so it can be arbitrarily complex. Ignore it, yes.

comment:21 in reply to:  16 Changed 5 years ago by Ralf Stephan

... 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 Matthias Köppe

Milestone: sage-8.2sage-9.4
Status: needs_infoneeds_work

comment:23 Changed 17 months ago by Matthias Köppe

Cc: Dave Morris added

comment:24 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:25 Changed 17 months ago by Matthias Köppe

Dependencies: #32139

comment:26 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:27 Changed 17 months ago by Matthias Köppe

Branch: u/mkoeppe/remove_function_call_syntax_for_symbolic_expressions

comment:28 Changed 17 months ago by Matthias Köppe

Authors: Punarbasu PurkayasthaPunarbasu Purkayastha, Matthias Koeppe
Commit: 1735601f1a8221ec9b97eac3ec2be11797818318

New commits:

4191337src/sage/all.py: Repair the filterwarnings regexp that ensures sagelib warnings are not ignored
6492b8csrc/sage/symbolic/ring.pyx: Pass the correct stacklevel to deprecatio()
04fdda6sage.all: In filterwarnings, remove leading '.*' from module regexes, use regex that matches vendored 'packaging'
a6691b4Merge #32139
9f5c197remove function call syntax from symbolic expressions
1735601SymbolicRing: Update documentation and tests

comment:29 Changed 17 months ago by Matthias Köppe

Cc: Nils Bruin Eric Gourgoulhon added
Status: needs_workneeds_review
Work issues: fix combinat/tutorial.py

comment:30 Changed 17 months ago by git

Commit: 1735601f1a8221ec9b97eac3ec2be11797818318fb48800b124fdfb1c9c43de368e687f0df973411

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

69a81casage.all: Fix up filterwarnings for collections.abc
fb48800Merge #32139

comment:31 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:32 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

patchbot found some doctest failures

comment:33 Changed 17 months ago by git

Commit: fb48800b124fdfb1c9c43de368e687f0df973411784782c27b42b9ebcf5126c93a61449fe36a477c

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

98f7af1src/sage/tests/books/computational-mathematics-with-sagemath/integration_doctest.py: Avoid deprecated use of an expression as a callable
784782cExpression: Deprecate arguments, args, number_of_arguments for non-callable expressions

comment:34 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:35 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:36 Changed 17 months ago by git

Commit: 784782c27b42b9ebcf5126c93a61449fe36a477c6566e8d64d1ad3059e5656e2ddd095f24515512b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6566e8dsrc/sage/combinat/tutorial.py: Remove use of expression call

comment:37 Changed 17 months ago by Matthias Köppe

Description: modified (diff)
Status: needs_workneeds_review

comment:38 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:39 Changed 17 months ago by git

Commit: 6566e8d64d1ad3059e5656e2ddd095f24515512bb12d1b022899b690b55e2e9f36d06e73ef2f1fa8

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

b12d1b0src/sage/symbolic/expression.pyx: Remove doctest of removed feature

comment:40 Changed 17 months ago by Matthias Köppe

fast_callable still needs updating

comment:41 Changed 17 months ago by Matthias Köppe

Dependencies: #32139#32139, #32233

comment:42 Changed 17 months ago by git

Commit: b12d1b022899b690b55e2e9f36d06e73ef2f1fa84063e39a16fc4a5f63060af5f339b0d73c1791db

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

94d7ce4PrimitiveFunction: Remove
945e802SFunction: Remove
905a234DeprecatedSFunction: Remove
48dff5bdeprecated_custom_evalf_wrapper: Remove
8167cb6Merge #32233
4063e39fast_callable: Convert expressions to callable symbolic expressions

comment:43 Changed 17 months ago by Matthias Köppe

Status: needs_workneeds_review

As fast_callable is used in plotting, this should be checked carefully for any regressions

comment:44 Changed 16 months ago by Matthias Köppe

Status: needs_reviewneeds_work
sage -t --long --random-seed=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/sage-patchbot/sage/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 718, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/sage-patchbot/sage/local/lib/python3.8/site-packages/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/sage-patchbot/sage/local/lib/python3.8/site-packages/sage/calculus/desolvers.py", line 1726, in desolve_odeint
        return desolve_odeint_inner(ivar)
      File "/home/sage-patchbot/sage/local/lib/python3.8/site-packages/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 Matthias Köppe

Dependencies: #32139, #32233#32139, #32233, #29738

comment:46 Changed 16 months ago by git

Commit: 4063e39a16fc4a5f63060af5f339b0d73c1791dbe198c5dcf743e97414d4de19e63770c059e9724c

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

ae0a28eExpression.__len__: Deprecate
65881e9Expression.__len__: Update doctest output
5690782ParametrizedSurface3D.__init__: Do not call len on something that could be either a variable or a list/tuple
3575a8fsage.calculus.desolvers.desolve_odeint: Simplify code, avoid calling __len__ on symbolic expressions
75d7751Merge #32139
4a63e61Trac #29738: update/fix doctest in asymptotics_multivariate_generating_functions.py
e40f9feMerge tag '9.4.beta5' into t/29738/perhaps_symbolic_expressions_should_not_have_a_length
75f6533desolve_odeint: Simplify code
86b0108Expression.number_of_operands, __len__: Update docstrings to use the word 'operands' instead of 'arguments'
e198c5dMerge #29738

comment:47 Changed 16 months ago by Matthias Köppe

Status: needs_workneeds_review

comment:48 Changed 16 months ago by Michael Orlitzky

Status: needs_reviewneeds_work

I think this needs a rebase onto rc0? Trac merge failed even though it rebases cleanly.

comment:49 Changed 16 months ago by git

Commit: e198c5dcf743e97414d4de19e63770c059e9724c9891a512409a4c03a3abc42a51f935ac001d58e6

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

9891a51Merge tag '9.4.rc0' into t/14270/remove_function_call_syntax_for_symbolic_expressions

comment:50 Changed 16 months ago by Matthias Köppe

Status: needs_workneeds_review

comment:51 Changed 16 months ago by Matthias Köppe

Dependencies: #32139, #32233, #29738
Reviewers: Matthias Koeppe, ...

comment:52 Changed 16 months ago by Travis Scrimshaw

Milestone: sage-9.4sage-9.5
Reviewers: Matthias Koeppe, ...Matthias Koeppe, Travis Scrimshaw
Status: needs_reviewpositive_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 soon-to-be-released 9.4 to get more testing.

comment:53 Changed 16 months ago by Matthias Köppe

Thank you!

comment:54 Changed 15 months ago by Volker Braun

Branch: u/mkoeppe/remove_function_call_syntax_for_symbolic_expressions9891a512409a4c03a3abc42a51f935ac001d58e6
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.