#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
- Branch set to u/nbruin/improve_printing_of_fderivative_by_adapting_the_appropriate_hook_in_pynac
comment:2 Changed 3 years ago by
- Commit set to 81ded03488eacfd2b4ba4c797a76351b9b2e00fc
comment:3 Changed 3 years ago by
- Cc rws added
comment:4 Changed 3 years ago by
- Cc bpage added
comment:5 Changed 3 years ago by
- Cc egourgoulhon added
comment:6 Changed 3 years ago by
- Commit changed from 81ded03488eacfd2b4ba4c797a76351b9b2e00fc to bce16a263458e5cbb363a058182ffd5642d68c7a
comment:7 Changed 3 years ago by
- 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
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
comment:10 Changed 3 years ago by
comment:11 follow-up: ↓ 13 Changed 3 years ago by
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
- Reviewers set to Bill Page, Eric Gourgoulhon
comment:13 in reply to: ↑ 11 Changed 3 years ago by
- 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
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
- 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
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
- Commit changed from bce16a263458e5cbb363a058182ffd5642d68c7a to 807d77dcf20bf563b7efaffd2e19ebea23988cff
comment:18 follow-up: ↓ 19 Changed 3 years ago by
- 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
comment:20 Changed 3 years ago by
- 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
- Commit 807d77dcf20bf563b7efaffd2e19ebea23988cff deleted
Excellent. Thanks.
After finally tracking down where the hook for this exists, the actual change only requires a few lines. With the attached POC branch:
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:
of course, actual printing styles can be adapted.
New commits:
trac 21286: Improved adaptive printing of FDerivative expressions