Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#12250 closed enhancement (fixed)

Implementation of weak k-tableaux

Reported by: aschilling Owned by: sage-combinat
Priority: major Milestone: sage-5.12
Component: combinatorics Keywords: tableaux, days49
Cc: sage-combinat, npgallup Merged in: sage-5.12.beta5
Authors: Anne Schilling Reviewers: Mike Zabrocki, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14519, #14101, #7983, #14772, #13589, #14907, #10630, #14143, #14015, #14516 Stopgaps:

Description (last modified by aschilling)

This patch implements a new class of weak k-tableaux. It also removes white spaces from core.py.

The methods on k-charge were contributed by Nate Gallup and Avi Dalal during Sage Days 49 in Orsay.

Attachments (1)

trac_12250-ktableaux-as.patch (91.0 KB) - added by aschilling 4 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 6 years ago by aschilling

  • Component changed from PLEASE CHANGE to combinatorics
  • Owner changed from tbd to sage-combinat

comment:2 Changed 4 years ago by aschilling

  • Dependencies #11742 deleted
  • Description modified (diff)
  • Keywords days49 added

comment:3 Changed 4 years ago by aschilling

  • Description modified (diff)
  • Summary changed from Implementation of weak and strong k-tableaux to Implementation of weak k-tableaux

comment:4 Changed 4 years ago by zabrocki

Strong tableaux was moved into ticket #14776

comment:5 Changed 4 years ago by zabrocki

  • Cc npgallup added

comment:6 Changed 4 years ago by aschilling

  • Reviewers set to Mike Zabrocki, Anne Schilling
  • Status changed from new to needs_review

comment:7 Changed 4 years ago by aschilling

  • Description modified (diff)

comment:8 Changed 4 years ago by aschilling

  • Dependencies set to #14519

comment:9 follow-up: Changed 4 years ago by tscrim

Hey Anne and Mike,

Thank you both for your work on this. However there are a few things I've noticed while glancing over the patch:

  • The inner bullet list on the INPUT: block for WeakTableau has one too many spaces, so it is overly indented. It needs to line up as follows:
    - item 1
    
      - inner item a
      - inner item b
    
    - item 2
    
  • k_charge_I() and k_charge_J() have the same docstring. In fact, it might be worthwhile to actually describe the algorithm.
  • I think it is worthwhile to have one abstract base class for all WeakTableau elements which has it's abstract methods defined. For example, you can also consolidate all of the more detailed explanations of the methods in there and link to them in their concrete classes. In particular, it because an easier/more straightforward to check to see if it's a WeakTableau and allow you an easy way to compare __eq__ between different representations of a weak tableau.
  • In the same vein, could we make it so that we can convert between different representations with parents? I.e. I'd like to do the following:
    sage: T = WeakTableaux(3, [5,2,1], [1,1,1,1,1,1])
    sage: T2 = WeakTableaux(3, [3,2,1], [1,1,1,1,1,1], representation='bounded')
    sage: T2(T[0])
    
  • Could we have a method like T.representation('bounded') which returns the corresponding parent using a different representation. Same on the elements (which of course would call the respective to_* method, but it would make it consistent IMO)?
  • I don't see an easy way to go to a factorized permutation from the other 2 representations. Right now it seems like I have to call
    WeakTableau_factorized_permutation.from_core_tableau().
    
  • I think the from_* (and quite possibly the to_*) methods maybe should be available in the parents rather than as classmethods for the element classes.
  • The .. SEEALSO:: block in k_charge() is not formatting correctly, so it doesn't link the methods.
  • Could you use the :arxiv:`1234.5678` autolink?
  • Remove self as an input (ex. in list_of_standard_cells())

Ack, that ended up being a longer list than I expected...

Best,
Travis

comment:10 Changed 4 years ago by zabrocki

  • Authors changed from Anne Schilling, Mike Zabrocki to Anne Schilling
  • Reviewers changed from Mike Zabrocki, Anne Schilling to Mike Zabrocki
  • Status changed from needs_review to needs_work

comment:11 in reply to: ↑ 9 Changed 4 years ago by aschilling

Hi Travis and Mike,

Thank you for your review comments. I incorporated them all, except for the input of the inner shape yet and the comments below:

  • In the same vein, could we make it so that we can convert between different representations with parents? I.e. I'd like to do the following:
    sage: T = WeakTableaux(3, [5,2,1], [1,1,1,1,1,1])
    sage: T2 = WeakTableaux(3, [3,2,1], [1,1,1,1,1,1], representation='bounded')
    sage: T2(T[0])
    

We discussed coercing during design discussions at Sage Days 49 and currently decided against it. For now, it is easy to go between the various representations with the representation method. If we really need this feature, we can always add coercion later.

  • I think the from_* (and quite possibly the to_*) methods maybe should be available in the parents rather than as classmethods for the element classes.

This is what Nicolas suggested to me, so I will leave it as is.

Best,

Anne

comment:12 follow-up: Changed 4 years ago by aschilling

  • Reviewers changed from Mike Zabrocki to Mike Zabrocki, Travis Scrimshaw
  • Status changed from needs_work to needs_review

comment:13 in reply to: ↑ 12 Changed 4 years ago by aschilling

Hi Mike,

Thanks for your review patch! I incorporated the changes. The new version now take as input

WeakTableaux(k, shape, weight, representation = 'core')

where shape is either a shape or a tuple of shapes in the skew case.

I hope everything looks ok now!

Anne

comment:14 follow-up: Changed 4 years ago by aschilling

  • Dependencies changed from #14519 to #14519, #14101

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by aschilling

Hi Mike,

The updated patch incorporated all changes we discussed (by e-mail). It

  • checks that the weight, shape, etc of the element agrees with its parent
  • checks for semistandardness
  • implements latex and pretty printing methods
  • makes some changes Nicolas suggested

The semistandard checks and pretty printing rely on #14101, so I put this as a dependency.

Best,

Anne

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by aschilling

Hi Mike,

I just posted a new version which incorporates your two doc review patches and fixes the bug in weights.

Anne

comment:17 in reply to: ↑ 16 Changed 4 years ago by aschilling

Fixed two more little issues that Mike found (spurious import of Tableau and innter_shape -> inner_shape).

Anne

comment:18 Changed 4 years ago by zabrocki

  • Status changed from needs_review to positive_review

comment:19 follow-up: Changed 4 years ago by zabrocki

I'm happy and all tests pass.

comment:20 in reply to: ↑ 19 Changed 4 years ago by aschilling

Replying to zabrocki:

I'm happy and all tests pass.

Great, thank you for the review! I just made height_of_restriced_subword a private method. Nothing else changed.

Anne

comment:21 Changed 4 years ago by zabrocki

I reaffirm positive review.

-Mike

comment:22 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

Changed 4 years ago by aschilling

comment:23 Changed 4 years ago by jdemeyer

  • Dependencies changed from #14519, #14101 to #14519, #14101, #7983, #14772, #13589, #14907, #10630, #14143, #14015, #14516

comment:24 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-pending

comment:25 Changed 4 years ago by aschilling

  • Milestone changed from sage-pending to sage-5.12

comment:26 follow-up: Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.12.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 in reply to: ↑ 26 Changed 4 years ago by aschilling

See http://trac.sagemath.org/ticket/15444 for a follow-up on the k-charge implementation.

Note: See TracTickets for help on using tickets.