Opened 7 years ago

Closed 7 years ago

#14040 closed enhancement (fixed)

Configurable "tall list" output style

Reported by: vbraun Owned by: jason
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:

Description (last modified by 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

Attachments (4)

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

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by vbraun

  • Summary changed from Configurable "tall list" output styl to Configurable "tall list" output style

comment:2 Changed 7 years ago by vbraun

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 7 years ago by vbraun

  • Authors set to Volker Braun
  • Description modified (diff)

comment:4 Changed 7 years ago by vbraun

  • Status changed from new to needs_review

comment:5 Changed 7 years ago by tscrim

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 7 years ago by nbruin

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 7 years ago by vbraun

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 7 years ago by vbraun

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

comment:9 Changed 7 years ago by kcrisman

  • Description modified (diff)

Changed 7 years ago by vbraun

Updated patch

Changed 7 years ago by vbraun

Updated patch

comment:10 Changed 7 years ago by vbraun

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

comment:11 Changed 7 years ago by vbraun

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 7 years ago by tscrim

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 7 years ago by vbraun

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 7 years ago by vbraun (previous) (diff)

comment:14 Changed 7 years ago by tscrim

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 7 years ago by vbraun

  • Dependencies set to #13685
  • Reviewers set to Travis Scrimshaw

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

Changed 7 years ago by vbraun

Rebased patch

comment:16 Changed 7 years ago by vbraun

  • Description modified (diff)

The last patch addresses your issues.

comment:17 Changed 7 years ago by vbraun

For the patchbot:

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

comment:18 Changed 7 years ago by tscrim

  • Status changed from needs_review to needs_work

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

comment:19 Changed 7 years ago by vbraun

  • Status changed from needs_work to needs_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 7 years ago by vbraun

Updated patch

comment:20 Changed 7 years ago by vbraun

  • Dependencies changed from #13685 to #13618

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

comment:21 Changed 7 years ago by tscrim

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

comment:22 Changed 7 years ago by vbraun

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 7 years ago by tscrim

  • Status changed from needs_review to positive_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 7 years ago by jdemeyer

  • Merged in set to sage-5.8.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.