Configurable "tall list" output style

[vbraun]

The displayhook pretty-prints lists/tuples of matrices in a readable way:

sage: m = random_matrix(GF(5),2)                                      
sage: [ m, m^2, m^3 ]  
[
[1 4]  [4 2]  [3 0]
[2 2], [1 2], [0 3]
]

But this is hardcoded to only apply to matrix types:

sage: MatrixGroup([m,m^2,m^3]).gens()
[[1 4]
[2 2], [4 2]
[1 2], [3 0]
[0 3]]

This ticket aims to add a _repr_option(key) method to parents to be able to return meta-information on the _repr_() output. In particular, _repr_option('element_ascii_art') will return a boolean that switches between the list display hooks.

I've also change the displayhook to switch to the list pretty printing if any entry is an ascii-art element, not just the first one. This upsets a handful of doctests, but is still quite low impact.

Apply

• attachment: trac_14040_housekeeping.patch
• attachment: trac_14040_repr_option.patch
• attachment: trac_14040_doctest.patch
• attachment: trac_14040_review.patch

Depends on #13618

Component: misc

Author: Volker Braun

Reviewer: Travis Scrimshaw

Merged: sage-5.8.beta2

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

[vbraun]

comment:2

I've also folded the is_atomic_repr() methods that some parents carried into this framework. Here is the method docstring for reference:

    def _repr_option(self, key):
        """
        Metadata about the :meth:`_repr_` output.

        INPUT:
        
        - ``key`` -- string. A key for different metadata informations
          that can be inquired about.

        Valid ``key`` arguments are:
        
        - ``'ascii_art'``: The :meth:`_repr_` output is multi-line
          ascii art and each line must be printed starting at the same
          column, or the meaning is lost.

        - ``'element_ascii_art'``: same but for the output of the
          elements. Used in :mod:`sage.misc.displayhook`.

        - ``'element_is_atomic'``: the elements print atomically, that
          is, parenthesis are not required when *printing* out any of
          `x - y`, `x + y`, `x^y` and `x/y`.

        OUTPUT:
        
        Boolean.

        EXAMPLES::

            sage: ZZ._repr_option('ascii_art')
            False
            sage: MatrixSpace(ZZ, 2)._repr_option('element_ascii_art')
            True
        """

[vbraun]

Author: Volker Braun

[tscrim]

comment:5

Since you're doing some cleanup, please forgive me nitpicking but you missed trac ticket #10467 on line 610 of parent.pyx and the coding convention page prefers .. TODO:: and .. NOTE:: blocks.

Best,

Travis

[nbruin]

comment:6

How badly is this going to interfere with classes that use string representations for hashing and comparison, both for time and for unexpected results? If a _repr_option gets changed (can they?) will element hash values suddenly change?

Also, I think there can be quite some exception raising in coercion discovery. Since error messages sometimes include operands, slowing down string conversions can possibly affect performance there too.

(That's a bit of a general problem: normally you'd think that by the time you're making string representations, you don't have to worry about basic speed any more. I'm not so sure that holds in Sage)

[vbraun]

comment:7

Nils, there is nothing user-configurable here. Its about metadata, not a configuration system. The only one who should change the _repr_option() output is the Parent author. This is also why its an underscore method.

For the patchbot who can't get the order right:

apply trac_14040_housekeeping.patch trac_14040_repr_option.patch trac_14040_doctest.patch

[vbraun]

comment:8

I've also fixed the docstrings in the first patch.

[vbraun]

Attachment: trac_14040_housekeeping.patch.gz

Updated patch

[vbraun]

Updated patch

[vbraun]

comment:10

Attachment: trac_14040_doctest.patch.gz

I've rebased the patches on top of sage-5.7.beta2. Older version will likely not work fyi.

[vbraun]

comment:11

Rebased to sage-5.7.beta4. Since this patch touches a couple of disparate places I would appreciate if somebody could review it so I don't have to spend my time rebasing it for every new beta.

[tscrim]

comment:12

Hey Volker,

I have a few questions/comments:

• Just to make sure, these are options that belong to individual Parent subclasses, not a global setting, correct?

• There's some strange formatting of commas, for example on line 71 of quadratic_form__local_field_invariants.py:

( 
Quadratic form in 4 variables over Rational Field with coefficients:  
[ 1 0 0 0 ]                                                          #
[ * 3 0 0 ]                                                          #
[ * * 5 0 ]                                                          #
[ * * * 7 ]                                                          ,
<BLANKLINE>
[1 0 0 0]
[0 0 0 1]
) 

I've put the pounds # to signify where the whitespace ends.

• Could you put the None in code formatting: ``None`` on line 129 in displayhook.py.

It currently looks good to me otherwise.

Thank you,

Travis

[vbraun]

comment:13

The _repr_option() is meant for a particular parent, though its not a good idea to change it during the parent's lifetime (parents must be immutable). So in this sense its global, too.

The strange formatting for commas is because the displayhook treats ascii art as a square block. In this case, the width is set by "Quadratic form in 4 variables over Rational Field with coefficients:" and the comma is placed at the bottom right corner. It would probably be nicer if the displayhook would place the comma after the last non-whitespace character in the bottom row, but thats not the aim of this ticket.

[tscrim]

comment:14

Okay.

One last thing I noticed in the housekeeping on line 273:

.. TODO:: 

Eventually, category should be Sets() by default

I would like to see this as

.. TODO:: 

    Eventually, category should be :class:`Sets` by default.

In particular, this should be indented. Once this is done, then I'll set this to positive review.

Thank you,

Travis

[vbraun]

Reviewer: Travis Scrimshaw

[vbraun]

comment:15

I'm marking this ticket dependent on the "doctests for rings" patch since the two conflict.

[vbraun]

Dependencies: #13685

[vbraun]

Rebased patch

[vbraun]

comment:16

Attachment: trac_14040_repr_option.patch.gz

The last patch addresses your issues.

[vbraun]

comment:17

For the patchbot:

apply trac_14040_housekeeping.patch, trac_14040_repr_option.patch, trac_14040_doctest.patch, trac_14040_review.patch

[tscrim]

comment:18

The current patch does not apply for me (as well) on 5.7.beta3 with all the prerequisite patches.

[vbraun]

comment:19

It applies cleanly on sage-5.8.beta1. I think its pretty hopeless to try to collect all tickets since 5.7.beta3 that this would depend on.

[vbraun]

Updated patch

[vbraun]

Changed dependencies from #13685 to #13618

[vbraun]

comment:20

Attachment: trac_14040_review.patch.gz

Got the wrong number for the "doctests for rings" ticket...

[tscrim]

comment:21

Then do you want me to rebase #13685 over this ticket?

[vbraun]

comment:22

Apparently #13685 needs to be rebased anyways, this was the problem in with the patchbot. It would be nice if you can base it on this ticket, otherwise it'll depend on what Jeroen merges first.

[tscrim]

comment:23

I'll be rebasing #13685 on this ticket (once 5.8.beta2 is released). Everything looks good to me now.

Thank you,

Travis

[jdemeyer]

Merged: sage-5.8.beta2