Opened 10 years ago

Closed 10 years ago

#14040 closed enhancement (fixed)

Configurable "tall list" output style

Reported by: Volker Braun Owned by: Jason Grout
Priority: major Milestone: sage-5.8
Component: misc Keywords:
Cc: Merged in: sage-5.8.beta2
Authors: Volker Braun Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13618 Stopgaps:

Status badges

Description (last modified by Volker Braun)

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

Attachments (4)

trac_14040_housekeeping.patch (17.0 KB) - added by Volker Braun 10 years ago.
Updated patch
trac_14040_doctest.patch (15.2 KB) - added by Volker Braun 10 years ago.
Updated patch
trac_14040_repr_option.patch (35.8 KB) - added by Volker Braun 10 years ago.
Rebased patch
trac_14040_review.patch (5.3 KB) - added by Volker Braun 10 years ago.
Updated patch

Download all attachments as: .zip

Change History (28)

comment:1 Changed 10 years ago by Volker Braun

Summary: Configurable "tall list" output stylConfigurable "tall list" output style

comment:2 Changed 10 years ago by Volker Braun

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

comment:3 Changed 10 years ago by Volker Braun

Authors: Volker Braun
Description: modified (diff)

comment:4 Changed 10 years ago by Volker Braun

Status: newneeds_review

comment:5 Changed 10 years ago by Travis Scrimshaw

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

comment:6 Changed 10 years ago by Nils Bruin

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)

comment:7 Changed 10 years ago by Volker Braun

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

comment:8 Changed 10 years ago by Volker Braun

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

comment:9 Changed 10 years ago by Karl-Dieter Crisman

Description: modified (diff)

Changed 10 years ago by Volker Braun

Updated patch

Changed 10 years ago by Volker Braun

Attachment: trac_14040_doctest.patch added

Updated patch

comment:10 Changed 10 years ago by Volker Braun

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

comment:11 Changed 10 years ago by Volker Braun

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.

comment:12 Changed 10 years ago by Travis Scrimshaw

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

comment:13 Changed 10 years ago by Volker Braun

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.

Last edited 10 years ago by Volker Braun (previous) (diff)

comment:14 Changed 10 years ago by Travis Scrimshaw

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

comment:15 Changed 10 years ago by Volker Braun

Dependencies: #13685
Reviewers: Travis Scrimshaw

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

Changed 10 years ago by Volker Braun

Rebased patch

comment:16 Changed 10 years ago by Volker Braun

Description: modified (diff)

The last patch addresses your issues.

comment:17 Changed 10 years ago by Volker Braun

For the patchbot:

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

comment:18 Changed 10 years ago by Travis Scrimshaw

Status: needs_reviewneeds_work

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

comment:19 Changed 10 years ago by Volker Braun

Status: needs_workneeds_review

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.

Changed 10 years ago by Volker Braun

Attachment: trac_14040_review.patch added

Updated patch

comment:20 Changed 10 years ago by Volker Braun

Dependencies: #13685#13618

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

comment:21 Changed 10 years ago by Travis Scrimshaw

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

comment:22 Changed 10 years ago by Volker Braun

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.

comment:23 Changed 10 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

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

Thank you,
Travis

comment:24 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.8.beta2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.