Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve documentation of pretty_print #29136

Closed
mwageringel opened this issue Feb 1, 2020 · 39 comments
Closed

improve documentation of pretty_print #29136

mwageringel opened this issue Feb 1, 2020 · 39 comments

Comments

@mwageringel
Copy link

This ticket improves the documentation of pretty_print, which was previously missing from the documentation.

CC: @nbruin @mantepse

Component: refactoring

Author: Markus Wageringel

Branch/Commit: 264bd7c

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/29136

@mwageringel mwageringel added this to the sage-9.1 milestone Feb 1, 2020
@mwageringel
Copy link
Author

Branch: u/gh-mwageringel/29136

@mwageringel
Copy link
Author

New commits:

8fbeb3629136: add context manager for temporary display preferences
25517ec29136: change default display mode to plain for text-based backends

@mwageringel
Copy link
Author

Author: Markus Wageringel

@mwageringel
Copy link
Author

Commit: 25517ec

@vbraun
Copy link
Member

vbraun commented Feb 1, 2020

comment:2

lgtm, but what do you think about a shorter name for the context manager? local_display_preferences can probably be improved... how about display_context or just display? In particular the latter is evocative of %display, so display(text='ascii_art) would be the closest to %display text ascii_art. Only downside is a potential clash with the IPython display command, but then that isn't all that useful anyways and its clear from the import what is being used.

@nbruin
Copy link
Contributor

nbruin commented Feb 2, 2020

comment:3

display sounds perhaps a bit too active. I really associate it with print and show and would expect it to do something similar. Indeed in

with display(text=...):
  pretty_print(...)

it's not so bad, but I would still go for display_context to avoid all confusion.

@mwageringel
Copy link
Author

Changed commit from 25517ec to 7cc3a83

@mwageringel
Copy link
Author

comment:4

Thanks for the suggestions. I have renamed it to display_context. The previous name was indeed a bit unwieldy.

I have also fixed the doctest failure in the Mathematica interface that was found by the patchbot (diff).


New commits:

16b8b9a29136: add context manager for temporary display preferences
7cc3a8329136: change default display mode to plain for text-based backends

@mwageringel

This comment has been minimized.

@mwageringel
Copy link
Author

Changed branch from u/gh-mwageringel/29136 to u/gh-mwageringel/29136v2

@mwageringel
Copy link
Author

comment:5

Could someone review this please?

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 14, 2020

comment:6

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2020

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

2a9250329136: fix optional mathematica doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 21, 2020

Changed commit from 7cc3a83 to 2a92503

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Sep 5, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 15, 2021

comment:9

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 15, 2021
@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2021

comment:10

As of Sage 9.3.rc2, pretty_print() prints plain by default in a terminal

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 8, 2021

comment:11

Actually text preference of the display manager does not affect terminal ipython backend.

sage: dm = get_display_manager()                                                
sage: dm.preferences.text                                                       
sage: pretty_print(x^2)                                                         
x^2
sage: dm.preferences.text = 'latex'                                             
sage: pretty_print(x^2)                                                         
x^2
sage: latex(x^2)                                                                
x^{2}
sage: html(x^2)                                                                 
<html><script type="math/tex; mode=display">\newcommand{\Bold}[1]{\mathbf{#1}}x^{2}</script></html>

@mwageringel
Copy link
Author

comment:12

Replying to @kwankyu:

Actually text preference of the display manager does not affect terminal ipython backend.

Well, they do if set to ascii_art or unicode_art. As far as I can tell, this now behaves as it should. Thanks.

Is there an interest in the other changes on the current branch or should we close this ticket? Mainly, the branch adds a context manager for changing the display preferences and improves the documentation of pretty_print.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 10, 2021

comment:13

Replying to @mwageringel:

Replying to @kwankyu:

Actually text preference of the display manager does not affect terminal ipython backend.

Well, they do if set to ascii_art or unicode_art. As far as I can tell, this now behaves as it should. Thanks.

Right. I missed that.

Is there an interest in the other changes on the current branch?

I don't know. Possibly. At least you need to revise the ticket description, and explain what this ticket achieves in the changed situation.

@mwageringel
Copy link
Author

comment:18

Replying to @kwankyu:

How about outside of the doctesting framework? I myself do not use rich text output much. But if you do and find yourself frequently changing text preferences, doesn't it make sense to import display_context into the global namespace so that you do not need to import it in each session?

For interactive use, I would just use the magic %display unicode_art for instance, so adding display_context to the global namespace does not seem necessary. I have added that magic to my sage.init file and rarely change it. I have difficulty to think of a non-hypothetical use-case in library code, so I admit this is not so useful outside doctests.

@kwankyu
Copy link
Collaborator

kwankyu commented May 3, 2021

comment:19

For doctesting rich output textual representations, functions like ascii_art(), unicode_art() are commonly used. So I don't see a room for display_context there.

For the documentation of pretty_print, %display magic should be used rather than display_context since that is what Sage users use to change preferences. Since %display errors out in doctests, we need to tag it # not tested

In conclusion, I don't see any real necessity or benefit of display_context in Sage.

One other comment:

--- a/src/sage/repl/rich_output/pretty_print.py
+++ b/src/sage/repl/rich_output/pretty_print.py
@@ -4,7 +4,7 @@ The ``pretty_print`` command
 
 Works similar to the ``print`` function, except that it always tries
 to use a rich output for an object, as specified via the text display
-preference. If such a rich output is not available, it is falling back on the
+preference. If such a rich output is not available, it falls back on the
 plain text.
 
 EXAMPLES::

@mwageringel
Copy link
Author

comment:20

Yes, I agree.

Replying to @kwankyu:

For the documentation of pretty_print, %display magic should be used rather than display_context since that is what Sage users use to change preferences. Since %display errors out in doctests, we need to tag it # not tested

I have implemented this by hiding the actual change of the preferences in a TESTS: block which is not shown in the documentation. An alternative would be to use get_test_shell(), which is commonly done for such IPython specific features. This would have the advantage that it does not use # not tested, but makes the documentation a bit harder to understand.


New commits:

7f34fe529136: improve documentation of pretty_print

@mwageringel
Copy link
Author

Changed commit from 433a3de to 7f34fe5

@mwageringel
Copy link
Author

Changed branch from u/gh-mwageringel/29136v3 to u/gh-mwageringel/29136v4

@mwageringel

This comment has been minimized.

@mwageringel mwageringel changed the title context manager for display preferences and documentation improvements improve documentation of pretty_print May 3, 2021
@kwankyu
Copy link
Collaborator

kwankyu commented May 4, 2021

Reviewer: Kwankyu Lee

@kwankyu
Copy link
Collaborator

kwankyu commented May 4, 2021

comment:21

Okay. It seems that this is the best we can do right now. In the future, I hope we could remove the # not tested tag though.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2021

comment:22

Merge fails.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 26, 2021

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

264bd7cMerge tag '9.4.beta6' into #29136

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 26, 2021

Changed commit from 7f34fe5 to 264bd7c

@mwageringel
Copy link
Author

comment:24

Trivial merge, so I am setting this back to positive.

@vbraun
Copy link
Member

vbraun commented Aug 29, 2021

Changed branch from u/gh-mwageringel/29136v4 to 264bd7c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants