#14266 closed enhancement (fixed)
Pretty Console Output --> ascii art
Reported by: | elixyre | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | user interface | Keywords: | ascii-art |
Cc: | nthiery, VivianePons | Merged in: | sage-5.11.beta0 |
Authors: | Jean-Baptiste Priez | Reviewers: | Volker Braun, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #8703 #14203 #14574 | Stopgaps: |
Description (last modified by )
My patch creates a simple ascii-art module to manipule several structure: list,dict,tuple, linear expression:
sage: R = NonCommutativeSymmetricFunctions(QQ).R() sage: R[1]**5 R + R + R + R + R + R + R + R + R + R + * ** * *** * ** * **** * ** * * ** * * ** *** * * * * * * * ** * * * ** * * * * ** * R + R + R + R + R + R * *** * ** * ***** ** ** * *** **** ** ***
Apply:
Attachments (11)
Change History (89)
Changed 8 years ago by
comment:1 follow-up: ↓ 9 Changed 8 years ago by
- Cc nthiery added; nthiery@… removed
Whats the difference between _repr_
, _pretty_repr_normal_
, and _pretty_repr_term_
? I think there should be only one _repr_
, using ascii art unconditionally if printing the object benefits from it. Why would you want to turn it off? I don't see the need for a different display handler, we aleady have one that does handle ascii art.
comment:2 Changed 8 years ago by
I implement _pretty_repr_ to introduce a special ascii-art method (like in mupad) and I prefere let the method _repr_ to allow differents use. In general case, we can copy the ouput and re-use it in terminal (it is very usefull I think). Then I put a method _pretty_repr_term_ like we have _repr_term_ for a linear expression.
I saw your handle ascii art and I try to send you a mail to merge that.... (no??).
comment:3 Changed 8 years ago by
I saw your email but #14040 has already been merged. What I'm saying is, there should be more integration into the existing framework and less reinventing the wheel. Also nobody wants an ugly representation. You can't copy/paste matrices either. To get an input that recreates the object, we already have
sage: m = matrix(ZZ, [[0, 0, 0], [0, 0, 0], [1, 0, 0], [0, 1, 0], [0, 0, 1]]) sage: m [0 0 0] [0 0 0] [1 0 0] [0 1 0] [0 0 1] sage: sage_input(m) matrix(ZZ, [[0, 0, 0], [0, 0, 0], [1, 0, 0], [0, 1, 0], [0, 0, 1]])
comment:4 Changed 8 years ago by
How about a %magic to switch to the sage input representation to the displayhook, e.g.:
sage: %sage_input on sage: m = matrix(ZZ, [[0, 0, 0], [0, 0, 0], [1, 0, 0], [0, 1, 0], [0, 0, 1]]) sage: m matrix(ZZ, [[0, 0, 0], [0, 0, 0], [1, 0, 0], [0, 1, 0], [0, 0, 1]])
or perhaps both (if sage_input
differs from _repr_
)
comment:5 Changed 8 years ago by
I don't reinventing the wheel... Then I prefere a nice LaTeX representation too but with some objects the LaTeX compilation is very slow. Then thanks to the method
sage: sage_input(m)
I didn't know but... after try...
sage: R = NonCommutativeSymmetricFunctions(QQ).R() sage: a = R[1]**3; a R + R + R + R * ** * *** * * ** * sage: %pcp 'Pretty Console Print' is uninstalled sage: sage_input(a) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) <ipython-input-10-7eccf1bd1684> in <module>() ----> 1 sage_input(a) /home/priezjea/sage-5.8.beta4/local/lib/python2.7/site-packages/sage/misc/sage_input.pyc in sage_input(x, preparse, verify, allow_locals) 252 if not verify: 253 sib = SageInputBuilder(allow_locals=allow_locals, preparse=preparse) --> 254 return sib.result(sib(x)) 255 256 # In verify mode, we actually compute and verify the answer with /home/priezjea/sage-5.8.beta4/local/lib/python2.7/site-packages/sage/misc/sage_input.pyc in __call__(self, x, coerced) 540 return SIE_literal_stringrep(self, loc_name) 541 else: --> 542 raise ValueError, "Can't convert %r to sage_input form"%x 543 544 def preparse(self): ValueError: Can't convert R[1, 1, 1] + R[1, 2] + R[2, 1] + R[3] to sage_input form
and
sage: R[1]**3 R[1, 1, 1] + R[1, 2] + R[2, 1] + R[3]
can be easily be copy.
So I don't say I invented something... I just purpose something to work (In my case) easily with linear combination.
comment:6 Changed 8 years ago by
- Summary changed from Simple Ascii-Art to Fix sage_input and pretty print for NonCommutativeSymmetricFunctions, others.
So the bug is that NonCommutativeSymmetricFunctions
doesn't define its own Sage input representation (define _sage_input_
). The ascii art should be the default (and only) print representation.
comment:7 Changed 8 years ago by
I like the idea of a magic function "%sage_input on". And if every thing is ascii-art... I have some question:
1 - The _repr_ method return (and must return) a string and it is too poor to produce an efficient and usefull ascii-art output. We could image use some baseline notion or ... My opinion is an ascii-art output must be a special class like PrettyConsoleRepr? or whatever which can be improve by heritage. (That is not available with string.) --> That why my opinion is we don't have to use _repr_ but anything else.
2 - How to use your code to get (easily) a list, a dict or whatever of some objects with an ascii-art output ? Your code implement only a hack for Matrix (or it was before...) no? --> If it is true my code suggests a solution (may be a bad one...)
(Sorry for my very bad english)
comment:8 Changed 8 years ago by
- Cc VivianePons added
I am a bit new on this discussion, but I want to add my contribution.
I think having _pretty_repr_
as a plus for combinatorial objects is a good idea and useful. It is not the same as
_repr_
.
First, you don't want to change the repr of every combinatorial objects to have pretty prints, this will just break everything every where. (I am thinking of trees, Dyck paths, partitions...). Most objects already had a way being pretty printed, it was just not centralized and could not be used in linear combinations.
I actually think having the two _repr_
and
_pretty_repr_
is useful. I have started to work with Jean-Baptiste's patch and depending on what I want to do with the output, I use both the pretty prints and usal prints.
comment:9 in reply to: ↑ 1 Changed 8 years ago by
Replying to vbraun:
Whats the difference between
_repr_
,_pretty_repr_normal_
, and_pretty_repr_term_
? I think there should be only one_repr_
, using ascii art unconditionally if printing the object benefits from it. Why would you want to turn it off? I don't see the need for a different display handler, we aleady have one that does handle ascii art.
There is definitely a need for a configurable display. In combinatorics depending on the context we really need to be able to be able to print various object in several different ways: The simplest example is partitions. Sometimes you need to see:
(3,3,2,1,1)
sometime you need
### ### ## # #
or if you are french
# # ## ### ###
Another example is graphs. Currently we only have
Graph on 6 vertices
Probably many user will rather have
Graph on 6 vertices with 5 edges
Or to see the incidence matrix...
So I definitely see a need for configurable output.
Florent
comment:10 follow-up: ↓ 12 Changed 8 years ago by
I don't agree with your partition example: It should just print ascii-art (with some cutoff if the tableau gets too large) and provide a method to get the tuple-of-integers. Which you need anyways. And the French should get along with the program and get used to the same notation as the rest of the world ;-)
Seriously, we should first of all think about sane defaults that are as expressive as possible without being overly verbose. Just making everything configurable is a lame cop-out where you admit that you couldn't be bothered thinking about good defaults. If you want to see some other representation then you can always add an extra method. Why do you want a %magic to always print graphs as incidence matrix if you can just call the incidence_matrix()
method?
comment:11 Changed 8 years ago by
I would recommend using the GlobalOptions
that was setup in #13605 (you can look in partition.py
or tableau.py
for examples) to choose been what type of output you want on a global level. I also have some fairly generic code for ascii printing lists of arrays (it's currently sitting inside of #13872 and have been calling the specialized display functions _repr_diagram()
), which is what you'd want since you'd want to call partition._repr_
to be consistent with the options specified there. I could separate that out into a separate ticket or copy/paste it here if you'd like.
comment:12 in reply to: ↑ 10 Changed 8 years ago by
Replying to vbraun:
I don't agree with your partition example: It should just print ascii-art (with some cutoff if the tableau gets too large) and provide a method to get the tuple-of-integers. Which you need anyways. And the French should get along with the program and get used to the same notation as the rest of the world ;-)
Unforunately, I seems that many combinatorialist agree with me. Please se the beginning of the sage/combinat/partitions.py
files. There is a PartitionOptions
dictionary which allows one to configure various thing including the output.
comment:13 in reply to: ↑ description Changed 8 years ago by
new ascii art version.
That one is compatible with the Braun's displayhook.
It uses sympy to rewrite expression (or try to rewrite)::
sage: integral(exp(x^2)/(x+1), x) ⌠ ⎮ ⎛ 2⎞ ⎮ ⎝x ⎠ ⎮ ℯ ⎮ ───── dx ⎮ x + 1 ⌡
(FIXME:: For that I have to use utf-8 output for that I use a hack in *sage/all_cmdline.py*...)
I implement several pretty_repr for combinatorial object and plug that for free_module::
sage: R = NonCommutativeSymmetricFunctions(QQ).ribbon() sage: Tableaux.global_options(convention="french") sage: R[3,1,1] R *** * * sage: R[3,1,1] * R[2,1,2] R + R *** *** * * * *** ** * * ** ** sage: F = QSym.F() sage: F[2,1,2] F ** * **
The AsciiArt? module (old PrettyConsolRepr?) purpose a 'new' option of baseline.
comment:14 Changed 8 years ago by
- Status changed from new to needs_review
- Summary changed from Fix sage_input and pretty print for NonCommutativeSymmetricFunctions, others. to Pretty Console Output --> ascii art
comment:15 follow-up: ↓ 16 Changed 8 years ago by
Looks pretty nice! I'm fine with the functionality, but the documentation needs to be improved. Especially since you want to provide other developers a framework to enable ascii art output.
Some of the names are awkward in English. Pretty console representation means that the console is pretty, not the repr(esentation). It would be nice (and it would make writing doctests easier) if we install a global function analogous to repr()
to get the ascii art representation. I would suggest we call it display()
and the magic %display
(instead of %pcp
, unless you want to use it as a pun on the hallucinogenic drug).
Similarly, I don't like __pretty_repr__
. Its not a variant of __repr__
with pretty output since it returns a specialized object instead of a string. Its also not a Python builtin, so Sage coding style would use only single underscores. How about we call it _ascii_art_()
? Similarly, set_display
instead of set_pretty_repr
.
Also, since this should be a general framework we should add a default _ascii_art_
method (with suitable documentation to benefit developers) to SageObject
. For example (in sage_object.pyx
):
def _ascii_art_(self): ''' Return an ASCII art representation. To implement multi-line ASCII art output in a derived class you must override this method. Unlike :meth:`_repr_`, which is sometimes used for the hash key, the output of :meth:`_ascii_art_` may depend on settings and is allowed to change during runtime. For example, :meth:`~sage.combinat.tableau.Tableaux.set_display` can be used to switch the ASCII art of tableaux between different mathematical conventions. OUTPUT: An :class:`~sage.misc.ascii_art.AsciiArt` object, see :mod:`sage.misc.ascii_art` for details. EXAMPLES: You can use the :func:`~sage.misc.ascii_art.display` function to get the ASCII art representation of any object in Sage:: sage: display(integral(exp(x^2)/(x+1), x)) ⌠ ⎮ ⎛ 2⎞ ⎮ ⎝x ⎠ ⎮ ℯ ⎮ ───── dx ⎮ x + 1 ⌡ Alternatively, you can use the `%display on/off` magic to switch all output to ASCII art and back:: sage: tab = StandardTableaux(3)[2]; tab [[1, 2], [3]] sage: %display on sage: tab 1 2 3 sage: Tableau.set_display("normal") sage: tab +---+ | 3 | +---+---+ | 1 | 2 | +---+---+ sage: %display off TESTS:: sage: 1._ascii_art_() 1 sage: type(_) sage.misc.ascii_art.AsciiArt ''' from sage.misc.ascii_art import AsciiArt return AsciiArt(repr(self))
Finally, every function must be documented and doctested. Please follow http://www.sagemath.org/doc/developer/conventions.html#docstring-markup-with-rest-and-sphinx for the markup. For example, we use an OUTPUT:
block instead of @postcondition
.
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 8 years ago by
Replying to vbraun:
Some of the names are awkward in English. Pretty console representation means that the console is pretty, not the repr(esentation). It would be nice (and it would make writing doctests easier) if we install a global function analogous to
repr()
to get the ascii art representation. I would suggest we call itdisplay()
and the magic%display
(instead of%pcp
, unless you want to use it as a pun on the hallucinogenic drug).
I'm not a big fan of %display
, it's somewhat vague as a true/false. How about %ascii_art
or have %display
accept a string input such as "ascii_art"
incase we end up with more display types later on? (I think if you see ascii art, you're trippin' on some %pcp
O_o )
Similarly, I don't like
__pretty_repr__
. Its not a variant of__repr__
with pretty output since it returns a specialized object instead of a string. Its also not a Python builtin, so Sage coding style would use only single underscores. How about we call it_ascii_art_()
? Similarly,set_display
instead ofset_pretty_repr
.
I like the _ascii_art_()
. However I would rather this be converted into using the GlobalOptions
framework instead of a separate function and attribute (tableaux already has this, and I can do it for Dyck paths if preferred). We can also use this for partitions as well.
Also, since this should be a general framework we should add a default
_ascii_art_
method (with suitable documentation to benefit developers) toSageObject
.
+1
@Jean-Baptiste
I would also like to see AsciiArt
inherit from SageObject
as well, and subsequently use _repr_
instead of __repr__
(1 underscore instead of 2). Also I would like to see the trailing whitespace removed from the new lines you've added, but this is not too important. (I like your ascii art in the beginning, too bad it's not of a sage [okay, I'm done with bad puns for now, promise].)
Thanks,
Travis
comment:17 follow-up: ↓ 18 Changed 8 years ago by
It would be even better if default _ascii_art_
was deTeXed _latex_
, rather than just _repr_
.
I played with tex2mail
a bit and it was quite nice, although I never got to trying to include it into Sage.
comment:18 in reply to: ↑ 17 Changed 8 years ago by
Replying to novoselt:
It would be even better if default
_ascii_art_
was deTeXed_latex_
, rather than just_repr_
. I played withtex2mail
a bit and it was quite nice, although I never got to trying to include it into Sage.
There are other implementations of that idea, for example AsciiTeX. But that's not what this ticket is about.
comment:19 in reply to: ↑ 16 Changed 8 years ago by
Replying to tscrim:
I'm not a big fan of
%display
, it's somewhat vague as a true/false. How about%ascii_art
or have%display
accept a string input such as"ascii_art"
incase we end up with more display types later on?
Sounds good to me. %display ascii_art
and %display repr
, perhaps with aliases on
/off
for the lazy.
Changed 8 years ago by
comment:20 Changed 8 years ago by
New version with several tests for lot of methods... with AsciiArt
inherit SageObject
.
I purpose to use %display ascii_art
and display repr
to select one use.
I hope I don't forget anything.
Enjoy.
Thanks, Jean-Baptiste
comment:21 Changed 8 years ago by
I see you are developing your own whitespace style. Unfortunately, its a slap in Guido Rossum's face. Can you have a look at pep8 ( http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements)?
def display_magic_function( self, mode = 'ascii_art'): # no def display_magic_function(self, mode='ascii_art'): # yes
comment:22 Changed 8 years ago by
It is unreadable for you or/and you want and I must code like that???
comment:23 Changed 8 years ago by
It is not totally unreadable, but reading code that does not follow a consistent styles does require additional mental resources that are better used elsewhere. As annoying as it seems to be on the outset, there have been scientific studies that have found evidence for the benefits of a consistent code style. All large software projects have one, and Sage uses the Python code style.
Changed 8 years ago by
comment:24 Changed 8 years ago by
I hope it's ok now...
comment:25 Changed 8 years ago by
What do you mean by
.. TODO:: revenir dessus..
That method looks fine to me. Is there anything left that you want to change?
comment:26 Changed 8 years ago by
Sorry I forgot to delete this message. It was when I adapted the text to the code... I have nothing to change.
comment:27 follow-up: ↓ 29 Changed 8 years ago by
A few more things:
- There's still a few functions without doctests (this includes
_private_methods
). - Bullet points needs a blankline before them, in particular, the authors block should be
AUTHORS: - Jean-Baptiste ...
- Any block should be in all caps, such as
EXAMPLES::
- I'd prefer using triple double-quotes rather than triple single-quotes for consistancy, but this is not a big issue
- Your commit message should have the description on the first line:
trac_14266: new version of ascii art module and magic function associated.
- Very minor and I won't let it hold the patch up, but try not to include any extra trailing whitespace.
Thanks.
comment:28 Changed 8 years ago by
In fact, do not include the trac ticket number in the commit message. Jeroen's merger script will put the correct number there automatically. Just
New version of ascii art module and associated magic functions
comment:29 in reply to: ↑ 27 ; follow-up: ↓ 33 Changed 8 years ago by
Replying to tscrim:
A few more things:
- There's still a few functions without doctests (this includes
_private_methods
).
I added some test... but sometime when the method it just a capsule:
def meth(self): return self.other_meth()
or something similar, I just put see :meth:other_meth
.
- Bullet points needs a blankline before them, in particular, the authors block should be
AUTHORS: - Jean-Baptiste ...
I think it's okay now.
- Any block should be in all caps, such as
EXAMPLES::
I hope it's okay too.
- I'd prefer using triple double-quotes rather than triple single-quotes for consistancy, but this is not a big issue
I try to do that when it's not my files and I think (but I don't verify) it is consistant in mine.
- Your commit message should have the description on the first line:
New version of ascii art module and magic function associated.
In the last patch I put that and this one must work on 5.9.
Thanks, Jean-Baptiste
Changed 8 years ago by
comment:30 Changed 8 years ago by
I don't like your approach for changing the output, you are monkey-patching sage.misc.displayhook to change its behavior from another module. Thats usually a no-no in production code (unless you cannot control the source that you are patching, which is not the case here). The problem is that if anybody is going to edit sage.misc.displayhook later on, they will inadvertently break your construction since noting in the source code of the displayhook knowns that this is not the actual code that is used when Sage is running. The standard solution to associating state with a function is to use a functionoid, that is, a class with a __call__
method:
class FormatObj(): def __init__(self): self._display = False def set_ascii_art(self, ascii_art): self._display = bool(ascii_art) def __call__(self, obj): if self._display: ... else: .... format_obj = FormatObj()
As a bonus, your code then wouldn't need layers of indirection format_obj
-> pretty_format
-> simple_format_obj
.
comment:31 Changed 8 years ago by
I will try to do that but I'm not really sure to understand the good way...
I have an other problem. I think I don't load display_magic_func.py
at the good place and that fail all tests.
comment:32 Changed 8 years ago by
The %display
magic should be defined in sage.misc.sage_extenison
with an @line_magic
like the other magics. You shouldn't have to add anything to sage.all_cmdline
.
comment:33 in reply to: ↑ 29 ; follow-up: ↓ 34 Changed 8 years ago by
Replying to elixyre:
Replying to tscrim:
A few more things:
- There's still a few functions without doctests (this includes
_private_methods
).I added some test... but sometime when the method it just a capsule:
def meth(self): return self.other_meth()or something similar, I just put
see :meth:other_meth
.
The need doctests not just docstings (there are still non-trivial methods without tests). However if they are simple wrappers like that, just make it an alias:
other_meth = meth
- I'd prefer using triple double-quotes rather than triple single-quotes for consistency, but this is not a big issue
I try to do that when it's not my files and I think (but I don't verify) it is consistent in mine.
This is not done in other files you've modified (ex. tableau.py
), and again, to be consistent with the project as whole, I would recommended using double quotes. However in new files you've created, this is a very minor issue to me (i.e. I won't let it hold up a positive review).
Thanks,
Travis
Changed 8 years ago by
comment:34 in reply to: ↑ 33 Changed 8 years ago by
I make some modifications to use correctly displayhook.
I have several problems for make tests: I use the fact to be in a terminal to take the width of the window... excepted in test context it seems to be differents...
File "ascii_art.py", line 25, in sage.misc.ascii_art Failed example: ascii_art(integrate(n^2/(pi*x),x)) Expected: 2 n *log(x) --------- pi Got: stty: stdin isn't a terminal 2 n *log(x) --------- pi
There is an other points, if we use utf8 in the terminal, it is not utf8 during the tests...
Why don't pass sage to utf8?
Thanks, Jean-Baptiste
comment:35 Changed 8 years ago by
The doctesting should be mostly utf-8 clean, I think. See also #14153. What exactly is the problem?
The stty: stdin isn't a terminal comes because the controlling terminal isn't a terminal for doctests. You should set a fixed width (probably 80) independent of the actual terminal size during doctests, something like this:
def terminal_width(): import os, sys from sage.doctest import DOCTEST_MODE isatty = os.isatty(sys.stdout.fileno()) if DOCTEST_MODE or not isatty: return 80 import fcntl, termios, struct rc = fcntl.ioctl(int(0), termios.TIOCGWINSZ, struct.pack('HHHH', sys.stdout.fileno(), 0, 0, 0)) h, w, hp, wp = struct.unpack('HHHH', rc) return w
The "pcp" abbreviation is still floating around in a few places, can you replace those?
Changed 8 years ago by
comment:36 Changed 8 years ago by
- Dependencies set to #8703
Hi,
I'm really bad with mercurial but it seems to be good...
- Tests works,
- normally, all methods and functions pass test
I made that module for see quickly trees in terminal so I added _ascii_art
methods for trees. (depends of #8703).
And all "pcp" abbreviation are disappear.
Thanks,
Jean-Baptiste
comment:37 Changed 8 years ago by
Looks great! I just have some minor nitpicks:
- You can actually doctest
_terminal_width()
, it should return 80 in doctest mode. - The
revenir dessus
is still there - There is a big chunk of commented-out code in
misc/sage_extension.py
, you probably want to delete that, too. - Catch-all
except:
statements are not allowed in Sage (the user might have pressed Ctrl-C and a KeyboardException was raised). Jeroen's merger script will check for that...
Apart from that it is good to go.
comment:38 Changed 8 years ago by
- Type changed from task to enhancement
comment:39 Changed 8 years ago by
- Dependencies changed from #8703 to #8703 #14203
- Description modified (diff)
- Reviewers set to Volker Braun, Travis Scrimshaw
I've uploaded a review patch which makes a variety of changes, including:
- basing this on #14203
- removed internal functions which are called only once in
*_tree.py
- used the global options in
tableau.py
- renamed method in
dyck_word.py
- docstring formatting fixes
- takes care of Volker's comments
Volker, would you be willing review my review patch?
Thanks,
Travis
comment:40 Changed 8 years ago by
Patchbot:
Apply trac_14266_ascii_art_13_05_15_EliX-jbp.patch trac_14266-ascii_art-review-ts.patch
comment:41 Changed 8 years ago by
I get two test failures on sage-5.10.beta3:
sage -t --long devel/sage/sage/doctest/sources.py # 1 doctest failed sage -t --long devel/sage/sage/misc/ascii_art.py # 1 doctest failed
comment:42 Changed 8 years ago by
Fixed:
sage -t --long misc/ascii_art.py [116 tests, 4.35 s] sage -t --long doctest/sources.py [327 tests, 122.27 s] ---------------------------------------------------------------------- All tests passed! ----------------------------------------------------------------------
I also removed all trailing whitespace from misc/ascii_art.py
and fixed some minor docstring issues.
comment:43 Changed 8 years ago by
apply trac_14266_ascii_art_13_05_15_EliX-jbp.patch, trac_14266-ascii_art-review-ts.patch
comment:44 Changed 8 years ago by
Travis, your patch only applies with fuzz 2. Can you rebase it against the newest Sage beta?
comment:45 Changed 8 years ago by
Done. (There is probably a minor conflict over one of the tableau patches I had above it in my queue.)
comment:46 Changed 8 years ago by
For patchbot:
Apply: trac_14266_ascii_art_13_05_15_EliX-jbp.patch, trac_14266-ascii_art-review-ts.patch
comment:47 Changed 8 years ago by
- Dependencies changed from #8703 #14203 to #8703 #14203 #14574
I found that the fuzz was due to #14574, and is now based off of that patch.
For patchbot:
Apply: trac_14266_ascii_art_13_05_15_EliX-jbp.patch, trac_14266-ascii_art-review-ts.patch
comment:48 Changed 8 years ago by
The patchbot seems to have a bug that prevents it from seeing the first patch. I mentioned it to Robert.
comment:49 Changed 8 years ago by
Apply: trac_14266_ascii_art_13_05_15_EliX-jbp.patch trac_14266-ascii_art-review-ts.patch
comment:50 Changed 8 years ago by
Wait, now the patch applies without #14574 but when I add the new dependency then I get fuzz 2.
comment:51 Changed 8 years ago by
Maybe I uploaded the patch before I refreshed it...try this one. If not, then I guess we'll just remove #14574 as a dependency.
Apply: trac_14266_ascii_art_13_05_15_EliX-jbp.patch trac_14266-ascii_art-review-ts.patch
comment:52 Changed 8 years ago by
Now the review patch doesn't apply at all. Removing #14574 is also not an option since that patch has already been reviewed.
comment:53 Changed 8 years ago by
Sorry, accidentally applied against beta2 instead of beta3. The reviewer patch actually applies, but still only with fuzz 2.
comment:54 Changed 8 years ago by
Okay, this one should apply cleanly. If it doesn't, I'll give you my immortal soul. :P
comment:55 Changed 8 years ago by
Uh-oh, I still get fuzz 2 with sage-5.10.beta4. One immortal soul, please :-P
comment:56 Changed 8 years ago by
Now I'll have to go steal it back. It should me take precisely as long as I need to install 5.4.beta4
to get it to you.
comment:57 Changed 8 years ago by
Done. Sorry it took so long, I had to beat a guy at a fiddle contest for my immortal soul.
comment:58 Changed 8 years ago by
- Status changed from needs_review to positive_review
Looks good! You can have your soul back ;-)
comment:59 Changed 8 years ago by
I think most (all?) of the # not tested: works only in interactive shell
examples can actually be tested using a pexpect
interface, for example sage0
. I'm not going to set the ticket to needs_work for this, but it could be addressed in the future.
comment:60 Changed 8 years ago by
Hey Volker,
Thanks for doing the review and for returning my immortal soul. :D
Hey Jeroen,
I'm not quite sure what you mean, could you elaborate/give an example?
Thanks,
Travis
comment:61 Changed 8 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:62 follow-up: ↓ 65 Changed 8 years ago by
Needs to be rebased for sage-5.10.beta5
comment:63 Changed 8 years ago by
Doctest example:
sage: sage0.eval("%display ascii_art") '' sage: sage0.eval("var('i')") 'i' sage: print sage0.eval("sum(factorial(i)*x^i, i, 0, 10)") 10 9 8 7 6 5 4 3 3628800*x + 362880*x + 40320*x + 5040*x + 720*x + 120*x + 24*x + 6*x 2 + 2*x + x + 1
comment:64 Changed 8 years ago by
Or, even better, use a test shell (does not require a separately spawned Sage session):
sage: from sage.misc.interpreter import get_test_shell sage: shell = get_test_shell() sage: shell.run_cell('%display ascii_art') sage: shell.run_cell('var("i")') sage: shell.run_cell('sum(factorial(i)*x^i, i, 0, 10)')
and so on.
comment:65 in reply to: ↑ 62 Changed 8 years ago by
Replying to vbraun:
Needs to be rebased for sage-5.10.beta5
Are you sure? It applies perfectly fine on a first trial of sage-5.10.rc0
Changed 8 years ago by
comment:66 Changed 8 years ago by
- Status changed from positive_review to needs_work
I've uploaded a new version of the review patch which uses the test shell. Since this changes doctests, and we (might) need to be rebased to 5.10.beta5
(which I'm building right now), I'm setting this back to needs review.
comment:67 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:68 Changed 8 years ago by
- Status changed from needs_review to positive_review
Ok, applies to plain sage-5.10.beta5. It does conflict with #14523, though.
Changes look good to me.
comment:69 Changed 8 years ago by
- Status changed from positive_review to needs_work
I just noticed that the sympy output is dependent on the terminal width:
sage -t sage/misc/ascii_art.py ********************************************************************** File "sage/misc/ascii_art.py", line 45, in sage.misc.ascii_art Failed example: shell.run_cell('sum(factorial(i)*x^i, i, 0, 10)') Expected: 10 9 8 7 6 5 4 3 3628800*x + 362880*x + 40320*x + 5040*x + 720*x + 120*x + 24*x + 6*x <BLANKLINE> 2 + 2*x + x + 1 Got: 10 9 8 7 6 5 4 3 2 3628800*x + 362880*x + 40320*x + 5040*x + 720*x + 120*x + 24*x + 6*x + 2*x + x + 1 ********************************************************************** File "sage/misc/ascii_art.py", line 67, in sage.misc.ascii_art
comment:70 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:71 Changed 8 years ago by
Last patch disables sympy terminal width detection during doctesting.
If you are happy with it, set ticket to positive review.
Patchbot:
apply trac_14266_ascii_art_13_05_15_EliX-jbp.patch, trac_14266-ascii_art-review-ts.patch, trac_14266_terminal_width.patch
comment:72 follow-up: ↓ 73 Changed 8 years ago by
- Status changed from needs_review to positive_review
Looks good to me. Thanks Volker.
comment:73 in reply to: ↑ 72 Changed 8 years ago by
Replying to tscrim:
Looks good to me. Thanks Volker.
Beware though that trac_14266_terminal_width.patch introduces trailing whitespace.
comment:74 follow-up: ↓ 75 Changed 8 years ago by
If you don't like trailing whitespace then feel free to work on commit hooks that check for it.
comment:75 in reply to: ↑ 74 Changed 8 years ago by
Replying to vbraun:
If you don't like trailing whitespace then feel free to work on commit hooks that check for it.
The sage dev scripts will strip them away for us. Which is how I actually detected this for I got a conflict when importing stuff into sage-git ... Do whatever you wish here; I just thought I might as well report the information.
comment:76 Changed 8 years ago by
In case anyone cares, here is the version without trailing whitespaces I put in the Sage-Combinat queue to ease my playing with importing into git.
http://combinat.sagemath.org/patches/file/tip/trac_14266_terminal_width.patch
comment:77 Changed 8 years ago by
- Merged in set to sage-5.11.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:78 Changed 8 years ago by
the ascii_art doesn't work with notebook. I purpose this patch but it is may be not a good solution.
patch