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

Refactor show() methods, rename to pretty_print #17821

Closed
vbraun opened this issue Feb 21, 2015 · 43 comments
Closed

Refactor show() methods, rename to pretty_print #17821

vbraun opened this issue Feb 21, 2015 · 43 comments

Comments

@vbraun
Copy link
Member

vbraun commented Feb 21, 2015

show should be an alias for pretty_print, which should always tries to use a rich output for an object (falling back on the plain text), while the plain print function should always use the plain text for the object

Depends on #17234
Depends on #17980
Depends on #18028
Depends on #18032

CC: @novoselt @ohanar @dcoudert

Component: graphics

Author: Volker Braun

Branch: 41f0a57

Reviewer: Andrey Novoseltsev

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

@vbraun vbraun added this to the sage-6.6 milestone Feb 21, 2015
@vbraun
Copy link
Member Author

vbraun commented Feb 21, 2015

Changed dependencies from #/17234 to #17234

@vbraun
Copy link
Member Author

vbraun commented Mar 18, 2015

Changed dependencies from #17234 to #17234, #17980

@vbraun
Copy link
Member Author

vbraun commented Mar 18, 2015

comment:2

One general question is how to handle multiple arguments: pretty_print('integers = ', ZZ) is supposed to give one latex string as output. After thinking about it, I think multiple arguments should always be string/latex/asciiart-ified. So no graphical output if you run pretty_print('plot =', plot(sin)).

@kcrisman
Copy link
Member

comment:3

As long as current behavior for show does not change I guess this would be okay.

@vbraun
Copy link
Member Author

vbraun commented Mar 19, 2015

comment:4

In addition to pretty_print and show there is also view among the generically-named functions that sound like they produce shiny output. Though the latter does something different.

@ohanar
Copy link
Member

ohanar commented Mar 19, 2015

comment:5

Why not generalize what is currently in the pretty_print function. I.e. allow an OutputType to have an optional join method (or the like) and then join consecutive things as much as possible. I think it would be convenient to have pretty_print('plot =', plot(sin)) work.

@vbraun
Copy link
Member Author

vbraun commented Mar 21, 2015

comment:6

Whats the expected output? Latex with a tiny picture of the plot inserted? After the rich output hits you'd have to join latex with an animated gif image and a dvi file, that doesn't work. It only makes sense if you get uniform rich output.

I think I'll just preserve the current behavior of special-casing certain homogeneous lists (i.e.: only graphs and only 2d plots).

@vbraun
Copy link
Member Author

vbraun commented Mar 21, 2015

Changed dependencies from #17234, #17980 to #17234, #17980, #18028

@vbraun
Copy link
Member Author

vbraun commented Mar 21, 2015

Branch: u/vbraun/refactor_pretty_print

@vbraun
Copy link
Member Author

vbraun commented Mar 21, 2015

Commit: 42f4985

@vbraun
Copy link
Member Author

vbraun commented Mar 21, 2015

comment:9

Refactoring graphics is now #18033, already too much going on here.

This basically works now, except that it unearthed various broken stuff elsewhere that wasn't exercised before.


New commits:

8577e87Refactor pretty_print/show to use the rich output system
42f4985remove the unused and broken print_or_typeset

@vbraun
Copy link
Member Author

vbraun commented Mar 21, 2015

Changed dependencies from #17234, #17980, #18028 to #17234, #17980, #18028, #18032

@vbraun

This comment has been minimized.

@vbraun

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2015

Changed commit from 42f4985 to 70e4d3a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 21, 2015

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

70e4d3afix doctest in symbolic expression show()

@novoselt
Copy link
Member

novoselt commented Apr 2, 2015

comment:13

Will there be an option for inline/display formulas with this system?

Although it seems a little ugly to modify global preferences before output and restoring them afterwards - shouldn't there be some way to pass "current preferences" that take precedence over "default ones"?

@novoselt
Copy link
Member

novoselt commented Apr 7, 2015

comment:14

Since I had no desire to dig into old show mess, I've merged this into my branch for SageMathCell based on 6.6.rc1 and get

OSError: [calculus ] docstring of sage.symbolic.expression.Expression.show:14: WARNING: Block quote ends without a blank line; unexpected unindent.

while building documentation.

What is the long-term plan for pretty_print/show/view? I've always used show to get a plot out or a displayed formula and view for inline formulas (which could be on the same line as printed text). That was definitely not something intuitive/easily discoverable. Perhaps show can be reserved for graphics only (2D/3D/animated) and its use for other objects deprecated? While pretty_print with some keywords to control display/inline should be the only official way to display formulas, with its use for graphics deprecated? And view just gone?.. For the moment it seems that view is left as is and show = pretty_print after this branch.

@vbraun
Copy link
Member Author

vbraun commented Apr 7, 2015

comment:15

View does different things in SageNB and elsewhere... I'm in favor of cleaning things up to the effect that:

  • show == pretty_print shall be synonymous. The name does not suggest any different behavior, so it would bad UI to invent one just because.

  • deprecate view, replace it with latex(...).compile() which always generates PDF output. I've implemented the compile method in ipython notebook: show compile LaTeX inplace #18116.

@vbraun
Copy link
Member Author

vbraun commented Apr 7, 2015

comment:16

Whats the use case for inline equations? In the notebook the cell output is always analogous to displaymath in LaTeX terminology, right?

@vbraun
Copy link
Member Author

vbraun commented Apr 7, 2015

comment:17

PS: There is the restricted_output context manager to temporarily alter the accepted rich output types. You can use that to force a particular output type...

@novoselt
Copy link
Member

novoselt commented Apr 7, 2015

comment:18

To me it sounds a bit strange to pretty_print a plot or to show a formula, but I am not a native speaker. Although I don't see the point of having two names for the same functionality, so why not then stick to shorter show and drop pretty_print?

Use cases for inline equations:

  • they are flushed left rather than centered, which makes a difference in combined output blocks
  • they have (or had) different padding in notebook, so were more compact
  • they are of course more compact because they use different LaTeX style
  • they could continue the line after print "Cool formula: ",

So I think they definitely should be available as an option. Having show/view to pick the style was not cool, of course.

@vbraun
Copy link
Member Author

vbraun commented Apr 8, 2015

comment:19

I kind of like pretty_print since it makes it clear that it behaves like print. On the other hand various objects already have .show() methods. I haven't really made up my mind about which is best. We could keep both if they are aliases...

I guess we should use inline/text style if and only if there is adjacent stuff. So, for example, pretty_print(a, b) should use inline style and pretty_print(a); pretty_print(b) should use block/display style.

@novoselt
Copy link
Member

novoselt commented Apr 8, 2015

comment:20

Replying to @vbraun:

I guess we should use inline/text style if and only if there is adjacent stuff. So, for example, pretty_print(a, b) should use inline style and pretty_print(a); pretty_print(b) should use block/display style.

How do you decide? If I made some output with suppressed newline in one place and then do extra output in another, it is difficult to detect. Also, if my code outputs two formulas on two lines and one of them is long, it may not be good to use display style since the centered short formula may be off the screen without centering. On the other hand, I dislike inline fractions/sums/limits, so even side-by-side things may be better in displaystyle.

It seems to me that its better to have a single default (display is more natural), but have a simple way for the user to tweak things if desired without the need of constructing all LaTeX code manually.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 16, 2015

Changed commit from 70e4d3a to 41f0a57

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 16, 2015

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

6278235Merge tag '6.7.beta1' into #17821
41f0a57fix broken docstring

@vbraun
Copy link
Member Author

vbraun commented Apr 16, 2015

comment:22

IMHO this is ready for review

@vbraun
Copy link
Member Author

vbraun commented Apr 16, 2015

Author: Volker Braun

@vbraun
Copy link
Member Author

vbraun commented Apr 16, 2015

comment:23

Right now latex/mathjax support in Sage doesn't even have a concept of multiple display equations. Before we can improve that we would first have to write the necessary tools, i.e. latex strings that also know whether they are in math / inline / display mode.

@novoselt
Copy link
Member

comment:24

Two questions about

<html><script type="math/tex">\newcommand{\Bold}[1]{\mathbf{#1}}...

What is this <html> tag??? Looking at HTML tutorials suggests that the whole document is wrapped into it with <head>, <body> etc. inside of it. What's the point of nesting it without any structure inside?

Why do we need to define the \Bold command in every formula??? I am not clear why do we need it at all and even if we do for MathJax initialization - can't it be done once for all of them?

@vbraun
Copy link
Member Author

vbraun commented Apr 20, 2015

comment:25

I don't like the <html> tags either but thats what the old SageNB relied on. Note that only the display backend for doctests still prints it that way for backward compatibility, the html tags are not shown by the commandline display backend. We should get rid of that but it'll probably be a larger auto-generated patch so I'd rather do it in a separate ticket.

I also don't like the extra \Bold definition but then that should be fixed in the latex generator; It should only emit macro definitions that are actually used. The rich display stuff shouldn't try to fix up the deficiencies of the latex generator.

@novoselt
Copy link
Member

Reviewer: Andrey Novoseltsev

@novoselt
Copy link
Member

comment:26

OK - I am setting this to positive review on the basis of things looking reasonable and better than they used to. Hopefully with a merge in an early beta we will get enough people to try it and detect corner cases if there are any.

@vbraun
Copy link
Member Author

vbraun commented Apr 21, 2015

Changed branch from u/vbraun/refactor_pretty_print to 41f0a57

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2015

comment:28

Okay guys. Now whenever I do graphs.PetersenGraph() in Sage I get a plot of it. Is it because of this ticket ?

Some graphs that I work with are much too large to be plotted. Just typing 'g' in the command line (as I often do to check the number of vertices) now runs an algorithm that I have to kill.

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2015

Changed commit from 41f0a57 to none

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2015

comment:29

Try it for yourself:

sage: graphs.RandomGNP(500,.2) # wait forever

@vbraun
Copy link
Member Author

vbraun commented Apr 23, 2015

comment:31

Followup at #18289

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2015

comment:32

No Volker. The problem is not the speed. The problem is that this is not the desired behaviour of __repr__. Unless you want to do the same for everything in Sage and call .plot() on matrices, posets, crystals, i.e. everything that plots.

Please, set it back to its original behaviour.

Nathann

@vbraun
Copy link
Member Author

vbraun commented Apr 23, 2015

comment:33

This ticket is closed. Any discussion should go to #18289

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 23, 2015

comment:34

Volker, please do not do this again. If you force me to accept this new behaviour for graphs, which is totally unpractical for whoever uses them, I will have to write to sage-devel again and it will be the same, over and over.

Please. This change that you made changes the way we work with graphs, it makes our life more complicated and it is not a good thing. Plus you did this for graphs and graphs only, which is totally incoherent with the behaviour of other classes.

Let's keep it simple Volker, please set this behaviour back to what it was before, i.e. display the number of points in the graph, and its name. Please.

Nathann

@kcrisman
Copy link
Member

comment:35

(This was resolved at #18289.)

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Apr 29, 2015

comment:36

(This was resolved at #18289.)

Still not resolved.

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

4 participants