Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21286 closed enhancement (fixed)

Improve printing of FDerivative by adapting the appropriate hook in PyNaC

Reported by: nbruin Owned by:
Priority: major Milestone: sage-7.4
Component: symbolics Keywords:
Cc: rws, bpage, egourgoulhon Merged in:
Authors: Nils Bruin Reviewers: Bill Page, Eric Gourgoulhon, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 807d77d (Commits) Commit:
Dependencies: Stopgaps:

Description

As Bill Page points out on:

https://github.com/pynac/pynac/issues/187

PyNaC does provide a special hook for printing FDerivative expressions. The relevant routine py_print_fderivative lives in src/sage/symbolic/pynac.pyx, on line 583 (or thereabouts).

It look likes an expression D[*params](f)(*args) would get printed using a call of the form

py_print_fderivative(<id of f>,params,args)

so we'd have enough information to decide if args consist of distinct simple variables, in which case we could print it as

diff(f(*args),[args[i] for i in params])

or something nicer.

Change History (21)

comment:1 Changed 3 years ago by nbruin

  • Branch set to u/nbruin/improve_printing_of_fderivative_by_adapting_the_appropriate_hook_in_pynac

comment:2 Changed 3 years ago by nbruin

  • Commit set to 81ded03488eacfd2b4ba4c797a76351b9b2e00fc

After finally tracking down where the hook for this exists, the actual change only requires a few lines. With the attached POC branch:

sage: var('x,y'); function('f')
sage: diff(f(y,x),x, y)
diff(f(y, x), y, x)
sage: diff(f(x+y,x-y),x,y)
D[0, 0](f)(x + y, x - y) - D[1, 1](f)(x + y, x - y)

so we get more intuitive printing when argument positions are easily determined by variable names, and revert to unambiguous notation if not. For univariate we do have:

sage: diff(f(sin(x)),x)
cos(x)*diff(f(sin(x)), sin(x))

of course, actual printing styles can be adapted.


New commits:

81ded03trac 21286: Improved adaptive printing of FDerivative expressions

comment:3 Changed 3 years ago by rws

  • Cc rws added

comment:4 Changed 3 years ago by bpage

  • Cc bpage added

comment:5 Changed 3 years ago by egourgoulhon

  • Cc egourgoulhon added

comment:6 Changed 3 years ago by git

  • Commit changed from 81ded03488eacfd2b4ba4c797a76351b9b2e00fc to bce16a263458e5cbb363a058182ffd5642d68c7a

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

5d234d6trac 21286: improved latex typesetting of FDerivative
bce16a2trac 21286: fix doctests (and adjust some edge cases detected by doctests)

comment:7 Changed 3 years ago by nbruin

  • Status changed from new to needs_review

OK, I think this is a reasonable start that's ready to be merged. In many common cases, expressions containing an FDerivative operator will be printed in a way that's valid input (an appropriate "diff" expression). The latex should look nice in those cases as well.

Other options can be tried and bikeshedded later on follow-up tickets. This at least should alleviate the worst complaints about printing derivatives.

comment:8 Changed 3 years ago by bpage

Thank you! I checked out this ticket and am just starting to look at it... already I like it. Great work.

comment:9 Changed 3 years ago by nbruin

  • Authors set to nbruin

comment:10 Changed 3 years ago by nbruin

  • Authors changed from nbruin to Nils Bruin

comment:11 follow-up: Changed 3 years ago by egourgoulhon

I also gave a look at it; it looks very good (in particular I've checked the LaTeX output in this jupyter notebook). Thanks for this improvement! There remains to replace \partial by \mathrm{d} in the LaTeX output for the univariate case, but this might be deferred to some follow-up ticket.

A question: shouldn't the change be documentated by some doctest in src/sage/symbolic/pynac.pyx?

comment:12 Changed 3 years ago by egourgoulhon

  • Reviewers set to Bill Page, Eric Gourgoulhon

comment:13 in reply to: ↑ 11 Changed 3 years ago by rws

  • Reviewers changed from Bill Page, Eric Gourgoulhon to Bill Page, Eric Gourgoulhon, Ralf Stephan
  • Status changed from needs_review to positive_review

Replying to egourgoulhon:

A question: shouldn't the change be documentated by some doctest in src/sage/symbolic/pynac.pyx?

The changed doctests with the new output suffice absolutely.

As the output is deemed satisfactory by reviewers, the pynac.pyx looks good, and make ptestlong passes, I'll set positive now. Thanks for the work.

comment:14 Changed 3 years ago by nbruin

Thanks for the quick review. Once this is merged, we can close #6344 and possibly some other tickets.

Concerning printing a "\mathrm{d}" rather than "\partial": yes, I think that's a nice refinement to make in a follow-up ticket. That might also be a place to start thinking about when function names can be safely put in the numerator and whether such behaviour should be subject to some global state.

comment:15 Changed 3 years ago by egourgoulhon

  • Status changed from positive_review to needs_work

Have you noticed that the patchbot next_method plugin failed on src/sage/symbolic/pynac.pyx? Probably the builtin function next should be used instead of .next() in line 653.

comment:16 Changed 3 years ago by tscrim

While I doubt it will make a difference, it would be good to check that there are no conflicts with #21369.

comment:17 Changed 3 years ago by git

  • Commit changed from bce16a263458e5cbb363a058182ffd5642d68c7a to 807d77dcf20bf563b7efaffd2e19ebea23988cff

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

f3bfeb8.next() -> next(..)
807d77ddoc+test

comment:18 follow-up: Changed 3 years ago by nbruin

  • Status changed from needs_work to positive_review

OK, let's see if this fares better.

comment:19 in reply to: ↑ 18 Changed 3 years ago by egourgoulhon

Replying to nbruin:

OK, let's see if this fares better.

Thanks for the changes!

comment:20 Changed 3 years ago by vbraun

  • Branch changed from u/nbruin/improve_printing_of_fderivative_by_adapting_the_appropriate_hook_in_pynac to 807d77dcf20bf563b7efaffd2e19ebea23988cff
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 3 years ago by bpage

  • Commit 807d77dcf20bf563b7efaffd2e19ebea23988cff deleted

Excellent. Thanks.

Note: See TracTickets for help on using tickets.