Configurable "tall list" output style

[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

• trac_14040_housekeeping.patch​
• trac_14040_repr_option.patch​
• trac_14040_doctest.patch​
• trac_14040_review.patch​

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

[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

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

[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

[Volker Braun]

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

[Volker Braun]

Updated patch

[Volker Braun]

Updated patch

[Volker Braun]

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

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

[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

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

[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

[Volker Braun]

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

[Volker Braun]

Rebased patch

[Volker Braun]

The last patch addresses your issues.

[Volker Braun]

For the patchbot:

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

[Travis Scrimshaw]

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

[Volker Braun]

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.

[Volker Braun]

Updated patch

[Volker Braun]

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

[Travis Scrimshaw]

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

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

[Travis Scrimshaw]

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

Thank you,
Travis