Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17821 closed enhancement (fixed)

Refactor show() methods, rename to pretty_print

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.6
Component: graphics Keywords:
Cc: novoselt, ohanar, dcoudert Merged in:
Authors: Volker Braun Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: 41f0a57 (Commits, GitHub, GitLab) Commit:
Dependencies: #17234, #17980, #18028, #18032 Stopgaps:

Status badges

Description (last modified by vbraun)

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

Change History (36)

comment:1 Changed 6 years ago by vbraun

  • Dependencies changed from #/17234 to #17234

comment:2 Changed 6 years ago by vbraun

  • Cc ohanar added
  • Dependencies changed from #17234 to #17234, #17980

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)).

comment:3 Changed 6 years ago by kcrisman

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

comment:4 Changed 6 years ago by vbraun

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.

comment:5 Changed 6 years ago by ohanar

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.

comment:6 Changed 6 years ago by vbraun

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).

Last edited 6 years ago by vbraun (previous) (diff)

comment:7 Changed 6 years ago by vbraun

  • Dependencies changed from #17234, #17980 to #17234, #17980, #18028

comment:8 Changed 6 years ago by vbraun

  • Branch set to u/vbraun/refactor_pretty_print

comment:9 Changed 6 years ago by vbraun

  • Commit set to 42f4985c93ebf3afb3772ad2298673ba1db372da
  • Dependencies changed from #17234, #17980, #18028 to #17234, #17980, #18028, #18032
  • Description modified (diff)

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

comment:10 Changed 6 years ago by vbraun

  • Description modified (diff)

comment:11 Changed 6 years ago by git

  • Commit changed from 42f4985c93ebf3afb3772ad2298673ba1db372da to 70e4d3af7d4f26e4cf7fca155ccfe12193e6dae4

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

70e4d3afix doctest in symbolic expression show()

comment:12 Changed 6 years ago by novoselt

  • Cc novoselt added

comment:13 Changed 6 years ago by novoselt

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"?

comment:14 Changed 6 years ago by novoselt

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.

comment:15 Changed 6 years ago by vbraun

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 #18116.

comment:16 Changed 6 years ago by vbraun

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

comment:17 Changed 6 years ago by vbraun

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...

comment:18 Changed 6 years ago by novoselt

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.

comment:19 follow-up: Changed 6 years ago by vbraun

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.

comment:20 in reply to: ↑ 19 Changed 6 years ago by novoselt

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.

comment:21 Changed 6 years ago by git

  • Commit changed from 70e4d3af7d4f26e4cf7fca155ccfe12193e6dae4 to 41f0a571ed77dd4e9a5d8ca9fdfe8837799ff6e9

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

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

comment:22 Changed 6 years ago by vbraun

  • Authors set to Volker Braun
  • Status changed from new to needs_review

IMHO this is ready for review

comment:23 Changed 6 years ago by vbraun

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.

comment:24 Changed 6 years ago by novoselt

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?

comment:25 Changed 6 years ago by vbraun

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.

comment:26 Changed 6 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to positive_review

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.

comment:27 Changed 6 years ago by vbraun

  • Branch changed from u/vbraun/refactor_pretty_print to 41f0a571ed77dd4e9a5d8ca9fdfe8837799ff6e9
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 6 years ago by ncohen

  • Commit 41f0a571ed77dd4e9a5d8ca9fdfe8837799ff6e9 deleted

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

Last edited 6 years ago by ncohen (previous) (diff)

comment:29 Changed 6 years ago by ncohen

Try it for yourself:

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

comment:30 Changed 6 years ago by ncohen

  • Cc dcoudert added

comment:31 Changed 6 years ago by vbraun

Followup at #18289

comment:32 Changed 6 years ago by ncohen

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

comment:33 Changed 6 years ago by vbraun

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

comment:34 Changed 6 years ago by ncohen

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

comment:35 follow-up: Changed 6 years ago by kcrisman

(This was resolved at #18289.)

comment:36 in reply to: ↑ 35 Changed 6 years ago by ncohen

(This was resolved at #18289.)

Still *not* resolved.

Note: See TracTickets for help on using tickets.