Opened 14 months ago
Closed 11 months ago
#30750 closed enhancement (fixed)
Implementation of semistandard tableaux as pathtableaux
Reported by:  Bruce  Owned by:  Bruce 

Priority:  major  Milestone:  sage9.3 
Component:  combinatorics  Keywords:  cactus group, BenderKnuth 
Cc:  kdilks, mantepse, pfannerer, darij  Merged in:  
Authors:  Bruce Westbury  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  13885d1 (Commits, GitHub, GitLab)  Commit:  13885d110f96644d6d299253fe48f4a91442b4bc 
Dependencies:  #30870  Stopgaps: 
Description
This is an implementation of the abstract base class PathTableau?. This implementation is for semistandard tableaux, represented as a chain of partitions (essentially, the GelfandTsetlin pattern). This generalises the jeudetaquin operations of rectification, promotion, evacuation from standard tableaux to semistandard tableaux. The local rule is the BenderKnuth involution.
Change History (35)
comment:1 Changed 14 months ago by
 Keywords cactus group BenderKnuth added
 Owner changed from (none) to Bruce
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 14 months ago by
 Branch set to u/Bruce/semistandardpath
comment:3 Changed 14 months ago by
 Commit set to eb2c9c97dc714563bb6304fc4773b0ccb410ba6f
comment:4 Changed 14 months ago by
 Status changed from new to needs_review
comment:5 Changed 14 months ago by
 Cc kdilks mantepse pfannerer darij added
comment:6 Changed 14 months ago by
 Commit changed from eb2c9c97dc714563bb6304fc4773b0ccb410ba6f to a90f69e234ad3bb308ec5eede6c925ffdc3421ea
Branch pushed to git repo; I updated commit sha1. New commits:
a90f69e  Completed coverage

comment:7 Changed 14 months ago by
 Commit changed from a90f69e234ad3bb308ec5eede6c925ffdc3421ea to b83bd7d7b0b2a86dd92e7af60c4d4696103ce300
Branch pushed to git repo; I updated commit sha1. New commits:
b83bd7d  Removed trailing white spaces

comment:8 Changed 14 months ago by
 Commit changed from b83bd7d7b0b2a86dd92e7af60c4d4696103ce300 to 48fa41ff2a33a688cd5385cb1d1147a27a11b6f3
Branch pushed to git repo; I updated commit sha1. New commits:
48fa41f  Merge branch 'develop' into semistandardpath

comment:9 Changed 14 months ago by
First lines of docs should say "Convert", "Check", etc instead of "Converts", "Checks", etc
comment:10 Changed 13 months ago by
 Commit changed from 48fa41ff2a33a688cd5385cb1d1147a27a11b6f3 to 89ba5acb76b11120b45d141090867d18ae8cf81a
comment:11 Changed 13 months ago by
 Commit changed from 89ba5acb76b11120b45d141090867d18ae8cf81a to 57cf0fbc8a7cc5f8b9fbeb32d2bb23ed2a3bb058
Branch pushed to git repo; I updated commit sha1. New commits:
57cf0fb  Added verbose option

comment:12 Changed 13 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:13 Changed 13 months ago by
Could someone please explain why this has failed testing?
Thanks
comment:14 Changed 13 months ago by
Because the sage installation of the patchbot has a failing doctest. Nothing to do with your ticket. So far, nobody has taken up the task to fix this issue with pexpect and singular interface.
comment:15 Changed 13 months ago by
missing a double colon here
+ TESTS:
The following should not be indented (never indent after a single colon)
+ INPUT: + + * a sequence of partitions + * a sequence of lists/tuples + * a semistandard tableau + * a GelfandTsetlin pattern
comment:16 Changed 13 months ago by
This should take double backticks
+ Return `True` if `self` is skew.
Single backticks are for latex code (same as $)
comment:17 Changed 13 months ago by
Missing a final dot here:
+ Rectify `self`
comment:18 Changed 13 months ago by
 Commit changed from 57cf0fbc8a7cc5f8b9fbeb32d2bb23ed2a3bb058 to f72ef26b8a26d6a8e88dc70aadf19a43d0d79970
Branch pushed to git repo; I updated commit sha1. New commits:
f72ef26  Fixed documentation

comment:19 Changed 13 months ago by
 Commit changed from f72ef26b8a26d6a8e88dc70aadf19a43d0d79970 to fc66fcae93bdadfdb5d3b08533d59e26a73de8c2
Branch pushed to git repo; I updated commit sha1. New commits:
fc66fca  More corrections to doc strings

comment:20 Changed 13 months ago by
Thanks. Now the syntax of
+ The :class:`SemistandardSkewTableau is not implemented.
is missing the backtick after SemistandardSkewTableau
comment:21 Changed 13 months ago by
 Commit changed from fc66fcae93bdadfdb5d3b08533d59e26a73de8c2 to 4ca70e1840829f4ddb38d98a0bac3f40dbc95d10
Branch pushed to git repo; I updated commit sha1. New commits:
4ca70e1  A correction

comment:22 Changed 13 months ago by
A few minor comments:
I would rename SemistandardPath
to be SemistandardPathTableau
to make the name more explicit. Also SemistandardPaths
is grammatically wrong as you want SemistandardPathTableaux
.
You should add more explanation there for the INPUT:
, mainly stating it "Can be one of the following:". There also needs to be at least one test for each of these input types. Can it also take a skew semistandard tableau? I think it should be able to. How about a skew partition then regular partitions? (This one, I would say it is fine the leave off and if someone does give this, I would be a bit surprised.)
For check
, it would be good to have actual EXAMPLES::
.
local_rule
should have a 1line description, then the rest of it should be separated in another paragraph with more details.
Error messages should start with lowercase letters (this is a Python convention we try to follow).
In rectify()
, instead of using Partition(data)
, it would be faster to use _Partitions(data)
. Although it would be best to not actually construct partitions, but instead keep them as tuples. Similarly, instead of calling SemistandardPath
, you can do P.element_class(P, data)
, where P = self.parent()
.
if inner == None: +if inner is None:
return self[0] != () +return bool(self[0])
 lt = list(self)  if lt[0] == ():  lt = lt[1:]  lt.reverse() + if not lt[1]: + lt.pop() return GelfandTsetlinPattern([list(a) for a in lt])
In _test_jdt_promotion
, could you make the tryexcept
block more local to the line you expect to fail? It looks weird to have it around the actual test run.
General PEP8 stuff such as spaces
def rectify(self,inner=None,verbose=False): +def rectify(self, inner=None, verbose=False):
 for i in range(1,len(self)1):  if not all(r>=s for r,s in zip(self[i+1],self[i])): + for i in range(1, len(self)1): + if not all(r >= s for r, s in zip(self[i+1], self[i])):
(but not necessarily limited to these).
comment:23 Changed 13 months ago by
comment:24 Changed 13 months ago by
 Commit changed from 4ca70e1840829f4ddb38d98a0bac3f40dbc95d10 to 82ce2e7689dddc627396c9d76b3134802a559dda
comment:25 Changed 13 months ago by
 Commit changed from 82ce2e7689dddc627396c9d76b3134802a559dda to cf2be3b9a6bc7768b69e79e8bcfe957c23d2ec26
Branch pushed to git repo; I updated commit sha1. New commits:
cf2be3b  Edited doctests

comment:26 Changed 13 months ago by
 Reviewers changed from tscrim to Travis Scrimshaw
Thank you. A few other little things I noticed on a second pass:
 You need to add the file to the big combinat module list for the documentation. (Actually, we forgot this for the frieze patterns. You can add it in here too if you want, but perhaps another ticket.)
 I think this is better formatting since it is code input:
This is implemented by toggling the entries of the `i`th list. The allowed range for `i` is 0 < `i` < len(self)1 so any row except +This is implemented by toggling the entries of the ``i``th list. +The allowed range for ``i`` is ``0 < i < len(self)1`` so any row except
if lt[1] == ():
>if not lt[1]:
comment:27 Changed 13 months ago by
 Commit changed from cf2be3b9a6bc7768b69e79e8bcfe957c23d2ec26 to aa7526ff3a265e8d188a6a77e67cd1338e31fac2
Branch pushed to git repo; I updated commit sha1. New commits:
aa7526f  Minor edits

comment:28 Changed 13 months ago by
 Commit changed from aa7526ff3a265e8d188a6a77e67cd1338e31fac2 to 6b69171305945e3c3363b87ceea8325e1d5db7be
Branch pushed to git repo; I updated commit sha1. New commits:
6b69171  Added module to documentation

comment:29 Changed 13 months ago by
The doctests in is_integral
are not indented, which is causing the docbuild to fail.
comment:30 Changed 13 months ago by
This will also likely conflict with #30870 and should have that as a dependency.
comment:31 Changed 13 months ago by
 Commit changed from 6b69171305945e3c3363b87ceea8325e1d5db7be to 414ea304a4f6e1e635f01b014ff68feac3cdc5bd
Branch pushed to git repo; I updated commit sha1. New commits:
414ea30  Indented Examples in is_integral

comment:32 Changed 13 months ago by
 Dependencies set to #30870
comment:33 Changed 13 months ago by
 Commit changed from 414ea304a4f6e1e635f01b014ff68feac3cdc5bd to 13885d110f96644d6d299253fe48f4a91442b4bc
comment:34 Changed 13 months ago by
 Status changed from needs_review to positive_review
comment:35 Changed 11 months ago by
 Branch changed from u/Bruce/semistandardpath to 13885d110f96644d6d299253fe48f4a91442b4bc
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Started testing
More testing
More testing
More testing, TestSuite fails
Took out Partitions
Passed unit tests
All tests passed.