Opened 7 years ago

Closed 7 years ago

#12796 closed enhancement (fixed)

Allow more general evaluation of FDerivativeOperator

Reported by: nbruin Owned by: burcin
Priority: major Milestone: sage-5.0
Component: symbolics Keywords:
Cc: kcrisman, mjo Merged in: sage-5.0.beta14
Authors: Nils Bruin Reviewers: Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nbruin)

Currently:

sage: f=function("f");
sage: fx=f(x).diff(x).operator();
sage: fx
D[0](f)
sage: fx(x^2)
NotImplementedError: currently all arguments must be distinct variables

This can change.

Apply 12796.patch, sage-trac_12796-review.patch.

Attachments (2)

sage-trac_12796-review.patch (6.5 KB) - added by mjo 7 years ago.
Revert two chunks of the review patch per comments
12796.patch (10.0 KB) - added by jdemeyer 7 years ago.
Apply this patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by nbruin

With the attached patch we have:

sage: f=function('f')
sage: g=function('g')
sage: fx=diff(f(x),x).operator()
sage: fx(g(x))
D[0](f)(g(x))
sage: I=fx(g(x)).integrate(x)
sage: I
integrate(D[0](f)(g(x)), x)

but unfortunately:

sage: I.simplify()
TypeError: unable to make sense of Maxima expression 'integrate(at(diff(f(t0),t0,1),[t0=g(x)]),x)' in Sage

The problem here is in symbolic_expression_from_maxima_string. Maxima's syntax allows a sequence as an argument for multiple substitutions, but the string-based parser is unable to handle that. max_to_sr and sr_to_max do the trick already, which is why "I" can be defined in the first place. simplify still uses the strings-based conversion, hence the problem. Any experts on the parser of maxima strings want to weigh in? In short, the maxima expression

at( f(x,y,z), [x=u,y=v,z=w])

is valid, but is currently not accepted by the string-based parser.

comment:2 Changed 7 years ago by nbruin

  • Authors set to Nils Bruin
  • Status changed from new to needs_review

The problem of lists as arguments can be solved by changing the grammar that sage.misc.parser.Parser accepts: An argument now can also contain a list. This is necessary because the parser is meant to accept essentially maxima language grammar and the construction at(f(x,y),[x=1,y=2]) is grammatical in maxima. This means that some errors may change from being syntax errors to being semantic errors for certain uses of Parser, but none occur in the doc tests.

Another possibly controversial point is the name of the temporary variables used: now it's t0,t1,.... It's good to pick names that print reasonably, because they can end up on the screen (e.g., if you use latex(maxima(f(x^2).diff(x))) or show(maxima(f(x^2).diff(x))). and t seemed sufficiently neutral for that.

comment:3 Changed 7 years ago by nbruin

  • Description modified (diff)

comment:4 Changed 7 years ago by kcrisman

  • Cc kcrisman added

comment:5 follow-up: Changed 7 years ago by kcrisman

Just for reference - related, though only very distantly, are #6756 and #6480.

comment:6 in reply to: ↑ 5 Changed 7 years ago by nbruin

Quite closely related: #7401, which takes a similar approach but only implements the univariate case.

comment:7 Changed 7 years ago by kcrisman

Hmm, does that mean that the doctest in #7401 that caused it to never be merged will now not fail? That would be nice; it was really a shame that was never brought in.

comment:8 Changed 7 years ago by mjo

  • Cc mjo added

It'll take me a while to understand the patch, but combined with #12801 and the use of function('f')(x) this looks like it will save me a lot of trouble:

sage: f = function('f')                            
sage: f(x).diff(x)(x=1).substitute_function(f, sin)
cos(1)

I have a lot of code I can test it against.

comment:9 follow-up: Changed 7 years ago by mjo

The approach looks correct to me, I've just added a bunch of doctests and fixed some small issues:

  • A failing doctest due to an extra space.
  • The p_arg parser couldn't handle val = [x]
  • Logic duplicated in max_at_to_sage() and calculus.at()

I get two deprecation warnings from my spline code, but one of them occurs on the lines that #12801 modifies, so we can fix them there.

If the review patch is OK, the original is for me.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by nbruin

Replying to mjo:

The approach looks correct to me, I've just added a bunch of doctests and fixed some small issues:

  • A failing doctest due to an extra space.

Thanks! that was a typo that crept in.

  • The p_arg parser couldn't handle val = [x]

A nice idea, but have you tested what your solution does with "val = a = 1" ? I think the result will be (val,(a,1)), which is probably not what is intended (it should be a syntax error).

I did think about a p_list_or_expr which peeks to the next token and dispatches to p_list, p_expr etc. Note that we have to be careful with accepting tuples. Otherwise f( (a,1) ) and f( a=1 ) both get parsed to the same form.

One way to avoid that is to let p_arg parse a=1 to {a:1}. I don't think Parser ever generates dicts. However, by now we're solving non-existent problems. So perhaps simply not accepting tuples in p_arg (as it is now) is the simpler solution.

  • Logic duplicated in max_at_to_sage() and calculus.at()

Don't import sage.calculus into maxima_lib, because the reverse import is already in effect. I don't think a bit of logic duplication here is so problematic, since the input the routines in maxima_lib have to deal with is much better controlled than the other ones. maxima_lib has the potential for much better optimizations. So perhaps leaving it as it was is simplest and acceptable?

Thanks for your work.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by mjo

Replying to nbruin:

A nice idea, but have you tested what your solution does with "val = a = 1" ? I think the result will be (val,(a,1)), which is probably not what is intended (it should be a syntax error).


Indeed. I'll revert this and remove its doctest. It used to return ('val', a) or I would add a doctest for the syntax error.

  • Logic duplicated in max_at_to_sage() and calculus.at()

Don't import sage.calculus into maxima_lib, because the reverse import is already in effect. I don't think a bit of logic duplication here is so problematic, since the input the routines in maxima_lib have to deal with is much better controlled than the other ones. maxima_lib has the potential for much better optimizations. So perhaps leaving it as it was is simplest and acceptable?


We're doing a lazy_import of maxima_lib in calculus, though, or is there still a reason to avoid it? I'll just revert that too if so.

comment:12 in reply to: ↑ 11 Changed 7 years ago by nbruin

Replying to mjo:

  • Logic duplicated in max_at_to_sage() and calculus.at()

Don't import sage.calculus into maxima_lib, because the reverse import is already in effect. I don't think a bit of logic duplication here is so problematic, since the input the routines in maxima_lib have to deal with is much better controlled than the other ones. maxima_lib has the potential for much better optimizations. So perhaps leaving it as it was is simplest and acceptable?


We're doing a lazy_import of maxima_lib in calculus, though, or is there still a reason to avoid it? I'll just revert that too if so.

I know I had trouble with it before, with dummy_integral. You're right that the lazy import might have fixed the issue there.

My main argument against sharing the code would be to keep the logic simple. The routines symbolic_expression_from_maxima_string and max_to_sr both serve to translate maxima expressions to SR expressions and they do so independently by design (ideally, we can eventually rip out the entire string-based conversion). In this case we're finding that both need to translate at to a subs method call. In both cases, the translation is straightforward. If you find that the translation step is too involved to replicate, the proper place to fix it would be to add, for instance, an "at" method to SR that accepts input closer to what symbolic_expression_from_maxima_string and max_to_sr find. I don't think that's necessary (and it's nicer to keep SR as lean as possible).

Changed 7 years ago by mjo

Revert two chunks of the review patch per comments

comment:13 follow-up: Changed 7 years ago by mjo

No problem, I'm just trying to wrap my head around all of the symbolics code. I've reverted those two changes in the last patch.

comment:14 Changed 7 years ago by nbruin

  • Description modified (diff)
  • Reviewers set to Michael Orlitzky

comment:15 in reply to: ↑ 13 Changed 7 years ago by nbruin

OK! I'm happy with the reviewer patch. Thank you for writing such comprehensive documentation. I'll leave it to you to set the ticket to positive review if you're happy now as well.

comment:16 Changed 7 years ago by mjo

  • Status changed from needs_review to positive_review

comment:17 Changed 7 years ago by jdemeyer

Why was there no newline after the commit message?... fixed now.

Changed 7 years ago by jdemeyer

Apply this patch

comment:18 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.