#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 )
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)
Change History (28)
comment:1 Changed 5 years ago by
- Component changed from PLEASE CHANGE to combinatorics
- Owner changed from tbd to sage-combinat
comment:2 Changed 4 years ago by
- Dependencies #11742 deleted
- Description modified (diff)
- Keywords days49 added
comment:3 Changed 4 years ago by
- 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
comment:5 Changed 4 years ago by
- Cc npgallup added
comment:6 Changed 4 years ago by
- Reviewers set to Mike Zabrocki, Anne Schilling
- Status changed from new to needs_review
comment:7 Changed 4 years ago by
- Description modified (diff)
comment:8 Changed 4 years ago by
- Dependencies set to #14519
comment:9 follow-up: ↓ 11 Changed 4 years ago by
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 forWeakTableau
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()
andk_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 aWeakTableau
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 respectiveto_*
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 theto_*
) methods maybe should be available in the parents rather than as classmethods for the element classes. - The
.. SEEALSO::
block ink_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
- 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
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 theto_*
) 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: ↓ 13 Changed 4 years ago by
- 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
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: ↓ 15 Changed 4 years ago by
- Dependencies changed from #14519 to #14519, #14101
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 4 years ago by
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: ↓ 17 Changed 4 years ago by
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
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
- Status changed from needs_review to positive_review
comment:19 follow-up: ↓ 20 Changed 4 years ago by
I'm happy and all tests pass.
comment:20 in reply to: ↑ 19 Changed 4 years ago by
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
I reaffirm positive review.
-Mike
comment:22 Changed 4 years ago by
- Milestone changed from sage-5.11 to sage-5.12
Changed 4 years ago by
comment:23 Changed 4 years ago by
- Dependencies changed from #14519, #14101 to #14519, #14101, #7983, #14772, #13589, #14907, #10630, #14143, #14015, #14516
comment:24 Changed 4 years ago by
- Milestone changed from sage-5.12 to sage-pending
comment:25 Changed 4 years ago by
- Milestone changed from sage-pending to sage-5.12
comment:26 follow-up: ↓ 27 Changed 4 years ago by
- 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
See http://trac.sagemath.org/ticket/15444 for a follow-up on the k-charge implementation.
Strong tableaux was moved into ticket #14776