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 )
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)
Change History (28)
comment:1 Changed 7 years ago by
- Summary changed from Configurable "tall list" output styl to Configurable "tall list" output style
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
- Status changed from new to needs_review
comment:5 Changed 7 years ago by
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
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
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
I've also fixed the docstrings in the first patch.
comment:9 Changed 7 years ago by
- Description modified (diff)
comment:10 Changed 7 years ago by
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
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
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 indisplayhook.py
.
It currently looks good to me otherwise.
Thank you,
Travis
comment:13 Changed 7 years ago by
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.
comment:14 Changed 7 years ago by
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
- 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.
comment:17 Changed 7 years ago by
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
- 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
- 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.
comment:20 Changed 7 years ago by
- Dependencies changed from #13685 to #13618
Got the wrong number for the "doctests for rings" ticket...
comment:21 Changed 7 years ago by
Then do you want me to rebase #13685 over this ticket?
comment:22 Changed 7 years ago by
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
- 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
- Merged in set to sage-5.8.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
I've also folded the
is_atomic_repr()
methods that some parents carried into this framework. Here is the method docstring for reference: