Opened 11 years ago

Closed 10 years ago

#9370 closed enhancement (fixed)

customize printing of elements in CombinatorialFreeModules

Reported by: jhpalmieri Owned by: sage-combinat
Priority: major Milestone: sage-4.7
Component: combinatorics Keywords:
Cc: sage-combinat Merged in: sage-4.7.alpha4
Authors: John Palmieri, Christian Stump Reviewers: Franco Saliola, Christian Stump
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jhpalmieri)

The attached patch allows customization of printing of elements in the class CombinatorialFreeModuleElement. It also adds documentation to CombinatorialFreeModule spelling out these options (and all of the other options, I think).

Apply only trac_9370-module-elt-repr-all-in-one.patch.

Attachments (6)

trac_9370-module-elt-repr.patch (25.3 KB) - added by jhpalmieri 11 years ago.
trac_9370-module-elt-repr-suggestions-cs.patch (16.9 KB) - added by stumpc5 10 years ago.
trac_9370-module-elt-repr-third.patch (2.9 KB) - added by jhpalmieri 10 years ago.
apply on top of other two patches
trac_9370-module-elt-repr-third.v2.patch (9.8 KB) - added by jhpalmieri 10 years ago.
apply on top of other two patches instead of ...-third.patch
trac_9370-module-elt-repr-fourth.patch (862 bytes) - added by jhpalmieri 10 years ago.
apply on top of first 3 patches, for review only
trac_9370-module-elt-repr-all-in-one.patch (37.3 KB) - added by jhpalmieri 10 years ago.
apply only this patch

Download all attachments as: .zip

Change History (28)

comment:1 follow-up: Changed 11 years ago by saliola

  • Status changed from new to needs_work

Hello John,

I like your ideas. I have a few quick comments for you now.

Comments on the docstring for CombinatorialFreeModule:

    - ``basis_keys`` - list, tuple, family, set, etc. defining the 
      basis of this module

basis_keys provides the indexing set for the basis, not the basis itself.

    - ``element_class`` - the class of which elements of this module
      should be instances (optional, default None)

I understand that the default argument is None, but it would be nice to have a description of the class used in that case since elements are not instances of None.

    - ``prefix`` - prefix used for printing elements of this module
      (optional, default 'B').  With the default, a monomial indexed
      by 'a' would be printed as ``B['a']``.
 
    - ``latex_prefix`` - prefix used in the LaTeX representation of
      elements (optional, default same as 'prefix').  With the 
      default, a monomial indexed by 'a' would be printed as
      ``B_{a}``.  If this is the empty string, then don't print
      monomials as subscripts: the monomial indexed by 'a' would be
      printed as ``a``, or as ``[a]`` if ``latex_bracket`` is True.

It doesn't say here, but it seems that prefix and latex_prefix are supposed to be strings. Or anything that concatenates with a string (for example, I think any instance of LatexExpr? will work).

    - ``repr_bracket`` - whether to print a bracket when printing
      elements (optional, default True).  If it is one of "[", "(", or
      "{", use it and its partner as brackets.  If this is any other
      string, use it as both brackets.

Please describe what bracket gets used with the default option. Also, the code sets the default to None, not True.

And perhaps a method called print_options might be useful (instead of pointing the user to an "internal" attribute). With no arguments, it will return _print_options and otherwise it will set the appropriate option. For example,

sage: A.print_options(repr_bracket="|") # to change repr_bracket

What do you think?

comment:2 in reply to: ↑ 1 Changed 11 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Replying to saliola:

Hello John,

Thanks for your comments.

basis_keys provides the indexing set for the basis, not the basis itself.

Fixed.

element_class: I understand that the default argument is None, but it would be nice to have a description of the class used in that case since elements are not instances of None.

Fixed, I think.

It doesn't say here, but it seems that prefix and latex_prefix are supposed to be strings. Or anything that concatenates with a string (for example, I think any instance of LatexExpr? will work).

I've documented that they should be strings. It's true that a LatexExpr will work, but I don't think we need to spell that out.

repr_bracket: Please describe what bracket gets used with the default option. Also, the code sets the default to None, not True.

I've expanded on the explanation here.

And perhaps a method called print_options might be useful (instead of pointing the user to an "internal" attribute). With no arguments, it will return _print_options and otherwise it will set the appropriate option.

Good idea, I've implemented this.

comment:3 Changed 11 years ago by jhpalmieri

Another idea would be to allow repr_bracket to be a pair (list or tuple) of strings, one for the left side and one for the right. It wouldn't be hard to implement, but is it worth doing? Would the documentation start to get too complicated?

comment:4 follow-up: Changed 11 years ago by jhpalmieri

Another idea would be to allow repr_bracket to be a pair (list or tuple) of strings, one for the left side and one for the right. It wouldn't be hard to implement, but is it worth doing? Would the documentation start to get too complicated?

Here's a patch which implements this idea. I think it's probably worth doing: consider

sage: F = CombinatorialFreeModule(QQ, [1,2,3], prefix='x', repr_bracket=["_{", "}"])
sage: e = F.basis()
sage: e[2] + 5*e[3]
x_{2} + 5*x_{3}

comment:5 Changed 11 years ago by jhpalmieri

Note: these print option interact with unique representation in ways which might be surprising, but this was also true with F._prefix. I've added an example to the documentation illustrating this, starting at line 849.

comment:6 Changed 11 years ago by jason

ping to saliola: what do you think about the changes in response to your comments?

comment:7 Changed 11 years ago by saliola

I forgot about this ticket. Thanks for the ping, Jason.

I like the changes. As long as all the tests pass, it gets a positive review from me. (I don't really have the time this week to upgrade my Sage installation to test this; if someone else can do it beforehand, that would be great!)

comment:8 in reply to: ↑ 4 Changed 11 years ago by nthiery

Replying to jhpalmieri:

Another idea would be to allow repr_bracket to be a pair (list or tuple) of strings, one for the left side and one for the right. It wouldn't be hard to implement, but is it worth doing? Would the documentation start to get too complicated?

Here's a patch which implements this idea. I think it's probably worth doing: consider

sage: F = CombinatorialFreeModule(QQ, [1,2,3], prefix='x', repr_bracket=["_{", "}"])
sage: e = F.basis()
sage: e[2] + 5*e[3]
x_{2} + 5*x_{3}

+1

comment:9 Changed 11 years ago by nthiery

  • Status changed from needs_review to needs_work

Hi John,

Sorry for letting this patch rot ... I like it much though! Thanks! As you, I am bothered by the current interaction between unique representation and printing customization, but don't have a good solution at this point.

I just went briefly through it. Three minor suggestions:

  • The logic in print_options could be factored out using something along:
        for option in ['latex_prefix',...]:
            if option in kwds: 
                args = True 
     	    self._print_options[option] = kwds[option]
    
  • The documentation of set_print_options duplicates that of the constructor. Could one of them just refer to the other? for the options ... see .... Actually, maybe the constructor should call set_print_options?
  • Could we find something better than _repr_asterix? That is that would refer more explicitly to scalar multiplication? In MuPAD it was timesDot, but it's not great either.

comment:10 Changed 11 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Hi Nicolas,

Thanks for the feedback. I've changed the code in print_options the way you suggested, and I shortened the docstring there. I wasn't comfortable having the constructor call print_options, so I didn't change that.

I agree that "repr_asterisk" is a bad name, so I rethought the names. We already had "prefix" to which I added "latex_prefix". In parallel with this, I changed "repr_bracket" to "bracket" (to go with my addition, "latex_bracket"). That means that there is no reason for "repr_asterisk" to have "repr" in its name anymore. I changed that to "scalar_mult". I think this is better:

    - ``scalar_mult`` - string to use for scalar multiplication in
      the print representation (optional, default "*")

What do you think?

Changed 11 years ago by jhpalmieri

comment:11 follow-up: Changed 10 years ago by stumpc5

  • Reviewers set to Christian Stump

Hi,

I will fold #10852 in here, if that's okay.

I like the multiple options! I gonna have a more detailed look for a review; here are two first comments:

  1. Why do you handle the old prefix option extra? Wouldn't it be enough to keep the prefix method and return to print_options()[\'prefix\'], rather thank keeping _prefix. A complete doctest provided by the buildbot can then check if _prefix is used anywhere.
  1. in lines 155p, multiplication is set to ast. Isn't there a way to give this argument to the repr_lincomb method? Otherwise, we get the following:
    sage: A = CombinatorialFreeModule(QQ,['+','*'],scalar_mult='@@')
    sage: A.an_element()
    2@@B['@@'] + 2@@B['+']
    

Best, Christian

comment:12 in reply to: ↑ 11 Changed 10 years ago by jhpalmieri

Replying to stumpc5:

Hi,

I will fold #10852 in here, if that's okay.

I like the multiple options! I gonna have a more detailed look for a review; here are two first comments:

  1. Why do you handle the old prefix option extra? Wouldn't it be enough to keep the prefix method and return to print_options()[\'prefix\'], rather thank keeping _prefix. A complete doctest provided by the buildbot can then check if _prefix is used anywhere.

I think it was just to preserve backward-compatibility, but if you think it's safe to remove it, please go ahead.

  1. in lines 155p, multiplication is set to ast. Isn't there a way to give this argument to the repr_lincomb method? Otherwise, we get the following:
sage: A = CombinatorialFreeModule(QQ,['+','*'],scalar_mult='@@')
sage: A.an_element()
2@@B['@@'] + 2@@B['+']

Putting it in repr_lincomb would be better, I agree.

comment:13 follow-up: Changed 10 years ago by stumpc5

I added an additional patch containing my two suggested additional features above.

In there I made quite some changes in sage.misc.repr_lincomb which effects many files. So I want to see what the buildbot returns (as I have a segfault in sage.categories.semigroups._test_associativity, but also without having the patches applied, so I cannot do a complete test).

I also removed _prefix, thus I had to make changes as well in combinat.schubert_polynomial, in combinat.symmetric_group_algebra, and in combiat.combiatorial_algebra. Is that okay, or should I undo those changes and leave _prefix?

comment:14 in reply to: ↑ 13 Changed 10 years ago by stumpc5

Maybe this makes the buildbot testing the ticket again.

For the buildbot:

Apply trac_9370-module-elt-repr.patch, trac_9370-module-elt-repr-suggestions-cs.patch

comment:15 follow-up: Changed 10 years ago by jhpalmieri

I'm happy with the changes except for the following things: first, there are some typos and some ways I would reword things, and I've put up a patch to go on top of "trac_9370-module-elt-repr-suggestions-cs.patch" to make those changes.

Second, the change in the print_options method makes the code simpler, but it also modifies the behavior, and I'm not sure I like the change. If we want to keep the change, then we need to modify the documentation. The change is this: with my original patch, if you set prefix but not latex_prefix, then latex_prefix would become prefix rather than the default value of "B". With your patch, we have this behavior:

sage: A = HeckeAlgebraSymmetricGroupT(QQ, 3)
sage: A.an_element()
T[1, 2, 3] + 2*T[1, 3, 2] + 3*T[2, 1, 3] 
sage: latex(A.an_element())
B_{[1, 2, 3]} + 2B_{[1, 3, 2]} + 3B_{[2, 1, 3]}

Ideally, everyone would set latex_prefix when they set prefix, but if they don't, I think they should default to being the same.

Changed 10 years ago by jhpalmieri

apply on top of other two patches

comment:16 in reply to: ↑ 15 Changed 10 years ago by nthiery

Hi Christian, John,

First, thanks so much for improving CombinatorialFreeModule?'s!

Replying to jhpalmieri:

The change is this: with my original patch, if you set prefix but not latex_prefix, then latex_prefix would become prefix rather than the default value of "B". With your patch, we have this behavior:

sage: A = HeckeAlgebraSymmetricGroupT(QQ, 3)
sage: A.an_element()
T[1, 2, 3] + 2*T[1, 3, 2] + 3*T[2, 1, 3] 
sage: latex(A.an_element())
B_{[1, 2, 3]} + 2B_{[1, 3, 2]} + 3B_{[2, 1, 3]}

Ideally, everyone would set latex_prefix when they set prefix,

I would not even try to enforce setting latex_prefix. Whenever the prefix does not contains specific math code, it would be an unnecessary burden to have to set both latex_prefix and prefix.

but if they don't, I think they should default to being the same.

+1

comment:17 Changed 10 years ago by jhpalmieri

Here are two new patches. The first is a replacement for "trac_9370-module-elt-repr-third.patch", and the second is an all-in-one patch incorporating all changes.

These restore the old behavior of setting prefix vs. latex_prefix. In fact, now if you set prefix but not latex_prefix, then the latex_prefix option remains set to None, and whenever latex_prefix is used, if it's set to None, it just uses the prefix setting instead. This makes some of the code cleaner.

These patches also document tensor_symbol and latex_scalar_mult.

comment:18 Changed 10 years ago by jhpalmieri

  • Description modified (diff)

Just to be clear, the "v2" version of the third patch is for review purposes.

Apply only

  • trac_9370-module-elt-repr-all-in-one.patch

Changed 10 years ago by jhpalmieri

apply on top of other two patches instead of ...-third.patch

comment:19 Changed 10 years ago by jhpalmieri

The Steenrod algebra code at #10052 revealed a small bug in the _latex_ method: when replacing a trailing "* 1" in latex code via

if x[len(x)-l-1:] == ast_replace+"1": 
     return x[:len(x)-l-1] 

if ast_replace is the empty string, this will just remove any trailing 1, for example from "x \otimes 1" or from "31", etc. So I'm changing the test to

if l > 0 and x[len(x)-l-1:] == ast_replace+"1": 
     return x[:len(x)-l-1] 

so it only does the replacement if the length l of ast_replace is positive. (I also find the letter "l" hard to distinguish from the number "1", so I'm changing it to "ln". Then we can pretend it's the natural log and we're doing calculus instead of algebra.)

Changed 10 years ago by jhpalmieri

apply on top of first 3 patches, for review only

Changed 10 years ago by jhpalmieri

apply only this patch

comment:20 Changed 10 years ago by stumpc5

  • Reviewers changed from Christian Stump to Franco Saliola, Christian Stump
  • Status changed from needs_review to positive_review

I haven't found any further issues; as all tests pass, so it gets a positive review.

Best, Christian

comment:21 Changed 10 years ago by jhpalmieri

  • Authors changed from John Palmieri to John Palmieri, Christian Stump

comment:22 Changed 10 years ago by jdemeyer

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