Opened 4 years ago

Closed 3 years ago

#24894 closed enhancement (fixed)

add super RSK algorithm to sage

Reported by: gh-MareoRaft Owned by:
Priority: major Milestone: sage-9.0
Component: combinatorics Keywords: gsoc19, super RSK, super, RSK, Haiman
Cc: sage-combinat, tscrim, aschilling, darij, mantepse, bump, brubaker, ghseeli Merged in:
Authors: Matthew Lancellotti, Chaman Agrawal Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 986df06 (Commits, GitHub, GitLab) Commit: 986df060ee9c0453b670e6e974654580f787d8f3
Dependencies: #25070, #28228 Stopgaps:

Status badges

Description

New trac user here. I have implemented the super RSK algorithm (as described in https://arxiv.org/pdf/1711.00420v1.pdf). I would like to contribute it to sage.

I would first like to talk about where the code should be organizationally. There is already a combinat/tableau.py containing the Tableau object which has a traditional RSK bump method. There is also a combinat/rsk.py module with the complete RSK algorithm and 1-2 variants.

The interesting thing is that the rsk module does not actually use the bump method from the tableau module, but rather re-implements it. I believe this should be changed (to eliminate redundancy). Also, actual Tableau objects are not used in the algorithm, but rather a list of lists (until the very end, when they are converted to Tableau). I believe this also should be changed.

So my *proposal* for organization is to make these changes, and then add my own super_rsk_bump method to the Tableau object, and then add my own super rsk insertion option to the rsk module, which would run the super rsk algorithm and call the super_rsk_bump method on the Tableau as needed.

The code I have *currently* does *not* follow the above proposal because I wrote it before discovering the tableau module. But before I waste time reorganizing everything, I wanted to get your input. I will include the code I have now as an attachment, even though it is unfinished. It is a modification of the rsk module.

The super RSK doesn't use integers as tableau entries but rather an alphabet where some numbers have carat symbols over them and some numbers do not. You can see that I implemented this alphabet with a linear ordering as the class Letter. I'm wondering if this alphabet might already exist elsewhere on sage, so I don't make redundant code. Also, I'm wondering where I should put this class, since it doesn't seem to belong in the rsk module. Probably in its own dedicated file?

Thank you for your input. I will strive to follow your recommendations thoroughly.

Attachments (1)

myrsk.py (36.9 KB) - added by gh-MareoRaft 4 years ago.

Download all attachments as: .zip

Change History (73)

Changed 4 years ago by gh-MareoRaft

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

  • Cc sage-combinat tscrim aschilling darij mantepse bump brubaker added

Welcome to Sage.

+1 for adding this. I saw the paper and have wanted to compare it with the bumping procedure of Benkart-Kang-Kashiwara (https://arxiv.org/abs/math/9810092, Sec. 4.5), of which the author seems to be unaware (actually, I probably should e-mail him and ask...). I'm cc-ing a bunch of people who might be interested in this ticket.

IMO, this should go in the rsk module as it is an RSK(-type) variant. I believe the letters are the same as what was recently done for the shifted tableaux (#22921), so you probably could use those.

The interesting thing is that the rsk module does not actually use the bump method from the tableau module, but rather re-implements it. I believe this should be changed (to eliminate redundancy).

-1 the code within the rsk is much faster than using the Tableau.bump() (no intermediate objects in particular).

Also, actual Tableau objects are not used in the algorithm, but rather a list of lists (until the very end, when they are converted to Tableau). I believe this also should be changed.

-1 lists (of lists) are easier to meant to be manipulated.

Now while I understand why you want to break things into these localized functions, but you are passing far too much data around to do so. Also, my quick impression is that it is actually making the code more complicated because you are forcing yourself to work in either a row or column format. In particular, you are not using your data structure efficiently nor taking advantage of the semistandardness by pulling out columns into a list. It might be time to have each of the bumping rules be implemented as classes, but I think we should refactor this code first and see what we get before doing that level of work.

<shameless_plug>You might be interested in coming to SageDays@ICERM; there is funding available for participants.</shameless_plug>

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

Welcome to Sage!

Martin Rubey has recently implemented an insertion related to this in #23896. It is also planned to generalize this to the semistandard insertions #24659. He uses growth diagrams.

In the code for shifted tableau, Kirill internally allowed entries 3.5 etc for primed entries. Perhaps something similar would work for you?

comment:3 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by gh-MareoRaft

Replying to tscrim:

Since I am new, I will aim to follow whatever guidelines you give me:

NEW PLAN:

  1. I will leave the original *standard* RSK alone (with the exception of adding one or two "if insertion=='sRSK'" statements into the RSK function that will call my specialized sRSK algorithm). So nobody has to worry about the current functionality changing or becoming less efficient.
  1. I will leave my sRSK bump function in the rskmodule (I guess a user who wants to do a single bump on a Tableau can import my function and then do sRSK_bump(P.to_list(), Q.to_list()) or similar.
  1. The classes Letter, BiLetter?, and BiWord? will be moved to their very own module. (This brings a new question. I have viewed the ShiftedPrimedTableau? object from https://trac.sagemath.org/ticket/22921 and it does indeed use the same alphabet and total ordering as the letters I am using. However, that code doesn't implement a class for these letters, but rather preprocesses the input in some way. So once he is finished with his code, I would suggest that his preprocessing code can actually be moved into my module and then his code can call my module or something. But anyway, I don't see why our two codes should interfere until after they are both independently added to Sage, so as not to complicate matters.)

Note: To be clear, my code does *not* use Tableau objects currently. It was an idea, but since you do not recommend it, I will leave it as-is with direct list manipulation.

NEW QUESTION:

How do I submit my code changes? I have cloned the git repo. I'm assuming I should create a branch (but should I name it) and then add my commits to that branch. After that, how do I add my branch to this proposal?

comment:4 in reply to: ↑ 2 Changed 4 years ago by gh-MareoRaft

Replying to aschilling:

Does the generalization you speak of generalize the superRSK algorithm too? (Because then my code is useless)

comment:5 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by tscrim

Replying to gh-MareoRaft:

Replying to tscrim:

Since I am new, I will aim to follow whatever guidelines you give me:

Don't be afraid to propose changes. Just because we may not like them, it doesn't mean it isn't good to challenge our assumptions and some of them can be clear improvements.

NEW PLAN:

  1. I will leave the original *standard* RSK alone (with the exception of adding one or two "if insertion=='sRSK'" statements into the RSK function that will call my specialized sRSK algorithm). So nobody has to worry about the current functionality changing or becoming less efficient.
  1. I will leave my sRSK bump function in the rskmodule (I guess a user who wants to do a single bump on a Tableau can import my function and then do sRSK_bump(P.to_list(), Q.to_list()) or similar.

I think you should consider adding a class for super tableaux. That way you could start with a tableau and do a (series of) insertion(s) in a natural way and so the output has a clear object type. It will also make it much easier for performing the inverse as the inverse_RSK will know from the type of the input that it should do sRSK.

  1. The classes Letter, BiLetter?, and BiWord? will be moved to their very own module. (This brings a new question. I have viewed the ShiftedPrimedTableau? object from https://trac.sagemath.org/ticket/22921 and it does indeed use the same alphabet and total ordering as the letters I am using. However, that code doesn't implement a class for these letters, but rather preprocesses the input in some way. So once he is finished with his code, I would suggest that his preprocessing code can actually be moved into my module and then his code can call my module or something. But anyway, I don't see why our two codes should interfere until after they are both independently added to Sage, so as not to complicate matters.)

I am not sure where you're reading. There is a dedicated letter class class PrimedEntry(SageObject). The tableau class has to (pre)process its input into these letters, but that is a necessary condition. Moreover, #22921 has been merged into Sage and is available in the latest beta. Since there is another use for this alphabet, +1 on extracting that part and the necessary other components into a dedicated module.

Note: To be clear, my code does *not* use Tableau objects currently. It was an idea, but since you do not recommend it, I will leave it as-is with direct list manipulation.

+1 on this. Just return some tableau objects.

NEW QUESTION:

How do I submit my code changes? I have cloned the git repo. I'm assuming I should create a branch (but should I name it) and then add my commits to that branch. After that, how do I add my branch to this proposal?

I generally follow http://doc.sagemath.org/html/en/developer/manual_git.html and manually push/pull from the trac server and update the branches. You will need to add your ssh key to your trac account, and then push to the trac server. Once that is done, just set the branch field on this ticket and change to "needs_review". Let me know if you need more specific instructions.

ADDENDUM - You should upgrade to the latest beta (on the develop branch) and base your branch off of that.

Last edited 4 years ago by tscrim (previous) (diff)

comment:6 Changed 4 years ago by gh-MareoRaft

  • Branch set to u/gh-MareoRaft/ticket/24894/add-super-rsk-algorithm-to-sage

comment:7 Changed 4 years ago by git

  • Commit set to 3750eaa4118ec0ccbc64506d8ff782907dfc3077

Branch pushed to git repo; I updated commit sha1. New commits:

comment:8 Changed 4 years ago by git

  • Commit changed from 3750eaa4118ec0ccbc64506d8ff782907dfc3077 to 7972e9bfc6dfdc3a46f50c4f24c42d4e28ac4de6

Branch pushed to git repo; I updated commit sha1. New commits:

7972e9bimplement super RSK.

comment:9 in reply to: ↑ 5 Changed 4 years ago by gh-MareoRaft

Replying to tscrim:

Please see my commit. I left the original RSK algo unaltered (but I moved it into _RSK), even though my algo generalizes it so we could just as easily run my algo (_sRSK) only.

You can see I moved all my code pertaining to Letters, BiLetters?, and BiWords? into a separate file called alphabet.py (I will rename it appropriately soon). I guess this is the major stuff that needs to change now, because as you said there is already a PrimedEntry(SageObject) class that is related. As well as the related ShiftedPrimedTableau class from https://trac.sagemath.org/ticket/22921.

I will deal with making a SuperTableau? class after dealing with the Primed stuff.

NEW PROBLEM:

I don't know how to run the test runner on my code. I am within the git repo on my branch, and I run sage -t rsk.py, and this results in the error '_is_horizontal() is not defined' even though it is. I think this has to do with the fact that I haven't successfully run make to build my official-git-repo version of sage, even though I have sage installed elsewhere on my computer.

If I copy and paste my code (from both this file and the alphabet.py file into a single file myrsk.py elsewhere on my computer) and then run sage -t myrsk.py on THAT, I not only get past the error but with a few minor tweaks I get all tests passing.

comment:10 Changed 4 years ago by tscrim

You need to import the parts that are not included in the global namespace. Basically, copy and paste in your tests into a (fresh) Sage session, you will get the same errors. You will have to do a `from foo import bar type statement and add that to the top of each of your functions.

comment:11 Changed 4 years ago by ghseeli

  • Cc ghseeli added

comment:12 Changed 4 years ago by gh-MareoRaft

The PrimedEntry class does indeed serve the same purpose as the Letter class I had before, so I will be using the PrimedEntry? class.

I need to make a SemistandardPrimedTableau which is just a semistandard tableau whose entries are primed instead of integers. To achieve this, I will make a class called GeneralizedSemistandardTableau which is just like SemistandardTableau? but does not require the entries to be integers. Then SemistandardTableau? will become a subclass of the generalized class with the only alteration being the enforcement of integers, and likewise SemistandardPrimedTableau? will be a subclass of the generalized class with the only alteration being the enforcement of primed entries.

comment:13 Changed 4 years ago by gh-MareoRaft

  • Branch changed from u/gh-MareoRaft/ticket/24894/add-super-rsk-algorithm-to-sage to u/gh-MareoRaft/ticket/24894/add-super-rsk
  • Commit changed from 7972e9bfc6dfdc3a46f50c4f24c42d4e28ac4de6 to bdfb486532fa435df2c2a95b7ba378b6a5adfdab

New commits:

73298b8tests passing. unfortunately one has to 'make build' sage every time one alters the code, which makes testing slow.
bdfb486testing if git push works

comment:14 Changed 4 years ago by gh-MareoRaft

I have no idea when this __classcall_private__ method runs or why it exists, but here is what the SemistandardTableau class will look like as a subclass of GeneralizedSemistandardTableau:

class SemistandardTableau(GeneralizedSemistandardTableau):
  @staticmethod
  def __classcall_private__(self, t):
      super(SemistandardTableau, self).__classcall_private__(self, t)

      # Check entried are integers
      if not all(isinstance(c,(int,Integer)) for row in t for c in row):
          raise ValueError("entries must be integers"%t)

  def __init__(self, parent, t):
      super(SemistandardTableau, self).__init__(parent, t)

      # Check the entries of t are positive integers
      from sage.sets.positive_integers import PositiveIntegers
      PI = PositiveIntegers()
      for row in t:
          if any(c not in PI for c in row):
              raise ValueError("the entries of a semistandard tableau must be integers")

comment:15 Changed 4 years ago by tscrim

Sorry I didn't get a chance to comment on this ticket yesterday. Your proposed hierarchy in comment:12 sounds good.

So __classcall_private__ is there to take the input parameters and standardize them (e.g., take a list and make it into a tuple). For parents, this is so you cannot have multiple copies of the same set within Sage. (This is more towards deep, internal to Sage code reasons and more important for things like rings, but it does retain some uses in combinatorics.) In this instance, it is so we can use Tableau like a normal class, but it automatically creates the parent. However, you cannot super-call the __classcall_private__.

TL;DR You can just remove it, and I can add in the appropriate things once you finish the first version.

comment:16 follow-up: Changed 4 years ago by gh-MareoRaft

Should my functions in general not edit their inputs inplace? For example, should bump(p, q) leave its inputs alone and output a NEW p and q? I think the answer is yes to be consistent with other sage things, but I would like confirmation before I bother changing it.

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

Replying to gh-MareoRaft:

Should my functions in general not edit their inputs inplace? For example, should bump(p, q) leave its inputs alone and output a NEW p and q? I think the answer is yes to be consistent with other sage things, but I would like confirmation before I bother changing it.

It depends on where they are and their use. If it is, say, an internal function of RSK that the user should not see, then no, it can edit their input inplace (assuming you have copied it into a mutable format). If it is a bump method of the tableau class, then yet, it should not edit its input (i.e., for T.bump(), it should not edit T).

comment:18 Changed 4 years ago by git

  • Commit changed from bdfb486532fa435df2c2a95b7ba378b6a5adfdab to 0d8cac2c8d0a6791a555dccf7a303c1c1c036274

Branch pushed to git repo; I updated commit sha1. New commits:

0d8cac2integrated discussed changes. added super rRSK extraction.

comment:19 Changed 4 years ago by gh-MareoRaft

@tscrim:

Perhaps you would like to make the necessary changes in the tableau.py file now? I think you know what is going on there more than me. I think the code is really close now, and it may soon be time to just touch things up and address the little stuff.

comment:20 Changed 4 years ago by tscrim

Yea, I can do that. However, I probably won't get to it until Monday.

comment:21 Changed 4 years ago by gh-MareoRaft

I'm getting an import error for permutation. I *think* it's due to circular dependency structure. permutation depends on rsk, rsk depends on tableau, and tableau depends on permutation.

If this is the case, I would question why permutation depends on rsk. The RSK algorithm is performed on *tableau*, not on permutations. Arguably, if somebody wanted to perform rsk on a 'permutation', then they should be doing something like p = permutation([1,2,3]); Tableau(p).rsk.

comment:22 Changed 4 years ago by gh-MareoRaft

I realized that there are more restrictions on SuperTableau? than I originally thought. I will push a new commit shortly with changes to tableau.py that will use multiple inheritance so we can define all these different types of tableau easily.

comment:23 Changed 4 years ago by git

  • Commit changed from 0d8cac2c8d0a6791a555dccf7a303c1c1c036274 to 9664b20b2215340fc93917b5e6e68bbac72038b3

Branch pushed to git repo; I updated commit sha1. New commits:

9664b20Broke up Tableau definitions/restrictions using multiple inheritance.

comment:24 Changed 4 years ago by git

  • Commit changed from 9664b20b2215340fc93917b5e6e68bbac72038b3 to 687161d057b94fb472ce40cbadb90dc7b8749008

Branch pushed to git repo; I updated commit sha1. New commits:

687161dUse new SemistandardSuperTableau class in rsk module.

comment:25 Changed 4 years ago by git

  • Commit changed from 687161d057b94fb472ce40cbadb90dc7b8749008 to 6045a86c0622f6854ce58b8c4ac8f550adcf816e

Branch pushed to git repo; I updated commit sha1. New commits:

6045a86resolve permutation import error by moving RSK imports in the permutation module into the specific methods that need them.

comment:26 Changed 4 years ago by git

  • Commit changed from 6045a86c0622f6854ce58b8c4ac8f550adcf816e to 1e3ede236d294f51c0a42b55f2b8f29852d77c04

Branch pushed to git repo; I updated commit sha1. New commits:

1e3ede2fix typo error. All ready for you Travis!

comment:27 Changed 4 years ago by tscrim

I've been too busy to take a good look at this, but I just wanted you to know that I haven't forgotten about it.

comment:28 Changed 4 years ago by gh-MareoRaft

No problem!

comment:29 Changed 4 years ago by tscrim

As an update, Darij and I have been talking about refactoring the RSK as a whole to follow a similar design pattern as growth diagrams (see #25070). This had actually been on my mind for awhile (I think I mentioned this above). The current design just is not scaling. So I will be doing some experimenting based on your code. Please bear with me; I appreciate your patience.

comment:30 Changed 4 years ago by gh-MareoRaft

No problem, Travis. I'm sure you will do a very good job.

comment:31 Changed 3 years ago by gh-ChamanAgrawal

  • Branch changed from u/gh-MareoRaft/ticket/24894/add-super-rsk to u/gh-ChamanAgrawal/24894_superRSK
  • Commit changed from 1e3ede236d294f51c0a42b55f2b8f29852d77c04 to 830898f44fc0670ba71215df7deba995d79b240e
  • Keywords gsoc19 added

Last 10 new commits:

7246802Removed extra parameter from insertion()
02db216Merge branch 'u/gh-ChamanAgrawal/27852_refactorRSK' into u/gh-ChamanAgrawal/28058_dualRSK
89a749cMinor change in docs and insertion()
15934c5implemented forward part of coRSK
e03a2c9Merge branch 'u/gh-ChamanAgrawal/27852_refactorRSK' into u/gh-ChamanAgrawal/28058_dualRSK
1bc9fb9Resolved merge conflicts
54985f0Merge branch 'u/gh-ChamanAgrawal/28058_dualRSK' into startcoRSK
3d073b5Complete implementation of coRSK algorithm
ec00502Add docs and test for coRSK
830898fInitial version, implemented forward rule for superRSK

comment:32 Changed 3 years ago by git

  • Commit changed from 830898f44fc0670ba71215df7deba995d79b240e to 1f9f3197823761b07c2549fe77395433401f96d3

Branch pushed to git repo; I updated commit sha1. New commits:

1f9f319Started reverse_bumping

comment:33 Changed 3 years ago by git

  • Commit changed from 1f9f3197823761b07c2549fe77395433401f96d3 to f25ec683cc9a2c3fff2e081905dca5718c4a26b4

Branch pushed to git repo; I updated commit sha1. New commits:

f25ec68Implement backward rule

comment:34 Changed 3 years ago by git

  • Commit changed from f25ec683cc9a2c3fff2e081905dca5718c4a26b4 to 7a5947d70b79e5cf3ff353b15ad5780ce1f03af7

Branch pushed to git repo; I updated commit sha1. New commits:

ea246f3Remove default parameter from *_format_output
fb82602manual merge, throwing parts of the baby out with the bathwater
eaddf7dreviewing dual RSK and coRSK while removing check functionality
e2986a9Remove default parameter from *_format_output
f6147beMerge branch 'public/ticket/25070' into u/gh-ChamanAgrawal/24894_superRSK
7a5947dAdd documentation

comment:35 Changed 3 years ago by git

  • Commit changed from 7a5947d70b79e5cf3ff353b15ad5780ce1f03af7 to fcb35649fb1f6df36ccfd779300382393e139ba0

Branch pushed to git repo; I updated commit sha1. New commits:

fcb3564Add docs and tests, edit class SemistandardSuperTableau

comment:36 Changed 3 years ago by git

  • Commit changed from fcb35649fb1f6df36ccfd779300382393e139ba0 to 05712e073455b93b41e48e89d4798ba6cb759717

Branch pushed to git repo; I updated commit sha1. New commits:

4db2ceaMore tests
2154fa0Improve SemisatndardSuperTableau
05712e0Refactor superRSK and add StandardsuperTableau

comment:37 Changed 3 years ago by git

  • Commit changed from 05712e073455b93b41e48e89d4798ba6cb759717 to 5ba89b259fbfbb5aa5560fce2c2fe5b690ff08c9

Branch pushed to git repo; I updated commit sha1. New commits:

dd1d6dcAdd support for words and refactor
e6042fdMinor fixes
5ba89b2More doc and test changes

comment:38 Changed 3 years ago by git

  • Commit changed from 5ba89b259fbfbb5aa5560fce2c2fe5b690ff08c9 to e53e9a8e6dda1ea4d11228558614132145eae18c

Branch pushed to git repo; I updated commit sha1. New commits:

fd0065dpreprocess PrimedEntry in forward rule
288099bAdd preprocess in super tableax
5f302baAdd docs and test for super tableax
103c2fdComplete minimal semistandard and standard super tableaux
e53e9a8Merge branch 'temp_superRSK' into u/gh-ChamanAgrawal/24894_superRSK

comment:39 Changed 3 years ago by gh-ChamanAgrawal

  • Dependencies set to #25070

comment:40 Changed 3 years ago by git

  • Commit changed from e53e9a8e6dda1ea4d11228558614132145eae18c to 5b622ebe7e672e9ef62d194378bf52289ac4d0fb

Branch pushed to git repo; I updated commit sha1. New commits:

05cb2f0Separate superTableau from tableau
5b622ebChange doctest

comment:41 Changed 3 years ago by git

  • Commit changed from 5b622ebe7e672e9ef62d194378bf52289ac4d0fb to ff3829df93e1283c994fd2ca3e75d09a9080b574

Branch pushed to git repo; I updated commit sha1. New commits:

ff3829dDoc changes

comment:42 Changed 3 years ago by gh-ChamanAgrawal

  • Status changed from new to needs_review

comment:43 Changed 3 years ago by git

  • Commit changed from ff3829df93e1283c994fd2ca3e75d09a9080b574 to 90386bddc806c6f49545daf6725813dfcff82518

Branch pushed to git repo; I updated commit sha1. New commits:

90386bdMinor correction

comment:44 Changed 3 years ago by git

  • Commit changed from 90386bddc806c6f49545daf6725813dfcff82518 to 2216bc15c284f14c21cf66c495a54667090602a1

Branch pushed to git repo; I updated commit sha1. New commits:

2216bc1Minor correction 2

comment:45 Changed 3 years ago by git

  • Commit changed from 2216bc15c284f14c21cf66c495a54667090602a1 to c9bc3dc945a0f2f2764dc53038e69b24c581b54f

Branch pushed to git repo; I updated commit sha1. New commits:

161db3dMinor doc change
98bd5a8Minor doc changes
4c18e17doc typos pointed out by Travis (mostly)
bfb6b5dMerge branch 'public/ticket/25070' of git://trac.sagemath.org/sage into public/ticket/25070
c05a591Fixing another `0`s and `1`s.
739aa17Fixing pyflakes warnings.
c9bc3dcMerge branch 'public/ticket/25070' into u/gh-ChamanAgrawal/24894_superRSK

comment:46 Changed 3 years ago by git

  • Commit changed from c9bc3dc945a0f2f2764dc53038e69b24c581b54f to d9090c329171acc91eb0c57c239736d6172ee8aa

Branch pushed to git repo; I updated commit sha1. New commits:

d9090c3Fix for python3 support

comment:47 Changed 3 years ago by gh-ChamanAgrawal

  • Dependencies changed from #25070 to #25070, #28228

comment:48 Changed 3 years ago by git

  • Commit changed from d9090c329171acc91eb0c57c239736d6172ee8aa to de9e7e7f27c91e2b8feb5d115c278fca9f6937e8

Branch pushed to git repo; I updated commit sha1. New commits:

6caca77Remove superRSK
eecee8dseparate superTableau from tableau
5d971dcadd super tableau to all.py
89eabd4Merge branch 'public/ticket/25070' into u/gh-ChamanAgrawal/28228_superTableau
24d3a0dDoc changes
224082eResolve merge conflict
4bdc3c0Correct pyflask patchbot error
de9e7e7Merge branch 'u/gh-ChamanAgrawal/28228_superTableau' into u/gh-ChamanAgrawal/24894_superRSK

comment:49 Changed 3 years ago by gh-ChamanAgrawal

  • Authors changed from Matthew Lancellotti to Matthew Lancellotti, Chaman Agrawal

comment:50 Changed 3 years ago by git

  • Commit changed from de9e7e7f27c91e2b8feb5d115c278fca9f6937e8 to 19617f840de3d0c4c28aacda04ac6e48f7f25a77

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

b6d6a95Add iterator(not working)
60ed353Debug complete and correct documentation
901d11dadd reference
55a2c43documentation change according to review
317ba34use standardTableau object for __iter__ and cardinality
f2290b0doc changes as per review
9d196c9Remove unused imports
71988ceChanges as per pep8 conventions
3019adeMerge branch 'u/gh-ChamanAgrawal/28228_superTableaux' into u/gh-ChamanAgrawal/24894_superRSK
19617f8documentation and test changes

comment:51 Changed 3 years ago by git

  • Commit changed from 19617f840de3d0c4c28aacda04ac6e48f7f25a77 to 6fc5e679526d77f33f336b47c14b41d0967bb84e

Branch pushed to git repo; I updated commit sha1. New commits:

6fc5e67minor changes

comment:52 follow-up: Changed 3 years ago by tscrim

  • Branch changed from u/gh-ChamanAgrawal/24894_superRSK to public/combinat/super_rsk-24894
  • Commit changed from 6fc5e679526d77f33f336b47c14b41d0967bb84e to 0614dd7af5848068d849ed5967b72dfb00423273
  • Milestone changed from sage-8.2 to sage-8.9
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to needs_work

I have added a doctest for the inverse map, which is currently failing on a semistandard tableaux:

sage -t --warn-long src/sage/combinat/rsk.py
**********************************************************************
File "src/sage/combinat/rsk.py", line 1984, in sage.combinat.rsk.RuleSuperRSK
Failed example:
    RSK_inverse(P, Q, insertion=RSK.rules.superRSK)
Expected:
    [[1', 1, 2', 2, 3', 3', 3', 3], [3, 2', 3, 2, 3', 3', 1', 2]]
Got:
    [[1', 1, 2', 2, 3', 3', 3], [3, 2', 3, 2, 3', 1', 2]]

There is a missing [3', 3'] pair, so there is definitely a bug somewhere.

Also, I have added a few more doctests and done a little cleanup. I will do further review once the bug is fixed.


New commits:

e2bf12aMerge branch 'u/gh-ChamanAgrawal/24894_superRSK' of git://trac.sagemath.org/sage into u/gh-ChamanAgrawal/24894_superRSK
0614dd7Some trivial reviewer changes and added some more tests.

comment:53 Changed 3 years ago by git

  • Commit changed from 0614dd7af5848068d849ed5967b72dfb00423273 to 297eac3df86a484e2362ab494df5327c04f5daa3

Branch pushed to git repo; I updated commit sha1. New commits:

297eac3remove row,column uniqueness in superRSK recording tableau

comment:54 in reply to: ↑ 52 Changed 3 years ago by gh-ChamanAgrawal

Replying to tscrim:

I have added a doctest for the inverse map, which is currently failing on a semistandard tableaux:

sage -t --warn-long src/sage/combinat/rsk.py
**********************************************************************
File "src/sage/combinat/rsk.py", line 1984, in sage.combinat.rsk.RuleSuperRSK
Failed example:
    RSK_inverse(P, Q, insertion=RSK.rules.superRSK)
Expected:
    [[1', 1, 2', 2, 3', 3', 3', 3], [3, 2', 3, 2, 3', 3', 1', 2]]
Got:
    [[1', 1, 2', 2, 3', 3', 3], [3, 2', 3, 2, 3', 1', 2]]

There is a missing [3', 3'] pair, so there is definitely a bug somewhere.

I have fixed it, it was because in superRSK the recording tableau columns can have repeated elements since we make upper row entries primed in case of column insertion.

comment:55 Changed 3 years ago by gh-ChamanAgrawal

  • Status changed from needs_work to needs_review

comment:56 Changed 3 years ago by git

  • Commit changed from 297eac3df86a484e2362ab494df5327c04f5daa3 to d376594ac9ebf9eb2ce07edb897fa6918bbb87b5

Branch pushed to git repo; I updated commit sha1. New commits:

d376594Finishing review and doing some doc standardization.

comment:57 Changed 3 years ago by tscrim

Thanks. I finished my review, and my changes include some other doc cleanup and standardization that I noticed needed to be done from doing the review. If my changes are good, then you can set a positive review.

comment:58 Changed 3 years ago by git

  • Commit changed from d376594ac9ebf9eb2ce07edb897fa6918bbb87b5 to 988ee0a178e79d3cf560785029ba78ccb5035360

Branch pushed to git repo; I updated commit sha1. New commits:

988ee0aA few more tweaks.

comment:59 Changed 3 years ago by git

  • Commit changed from 988ee0a178e79d3cf560785029ba78ccb5035360 to 52f8618a18e74f6f2d0637eec59be4acd7872b7e

Branch pushed to git repo; I updated commit sha1. New commits:

52f8618Removing one additional doctest for matrix input.

comment:60 Changed 3 years ago by git

  • Commit changed from 52f8618a18e74f6f2d0637eec59be4acd7872b7e to 31f44a992956652d6434c5019ab555af95adff20

Branch pushed to git repo; I updated commit sha1. New commits:

31f44a9Merge branch 'develop' into public/combinat/super_rsk-24894

comment:61 Changed 3 years ago by git

  • Commit changed from 31f44a992956652d6434c5019ab555af95adff20 to 8261edd40bed258971512f293c637ec5dc8de5ec

Branch pushed to git repo; I updated commit sha1. New commits:

8261eddFixing issues with bad merge of ref index and updating references.

comment:62 Changed 3 years ago by git

  • Commit changed from 8261edd40bed258971512f293c637ec5dc8de5ec to eb16f79f7fd912e6e965af420be7ccd22b7687b1

Branch pushed to git repo; I updated commit sha1. New commits:

eb16f79Merge branch 'develop' into public/combinat/super_rsk-24894

comment:63 Changed 3 years ago by gh-ChamanAgrawal

This looks good to me. If there are no patch bot errors, I think this can be a positive review.

Last edited 3 years ago by gh-ChamanAgrawal (previous) (diff)

comment:64 Changed 3 years ago by git

  • Commit changed from eb16f79f7fd912e6e965af420be7ccd22b7687b1 to c5aac2b10a7d12cf80003db5a443e33842ef2c30

Branch pushed to git repo; I updated commit sha1. New commits:

c5aac2bUpdating and fixing references to Muth.

comment:65 Changed 3 years ago by gh-MareoRaft

Travis and Chaman, would you mind adding my name also at the top of the rsk.py file, as it contains some of my code? I forgot to add it in my initial commits. By the way, this all looks really great. I had forgotten about this old ticket.

comment:66 Changed 3 years ago by git

  • Commit changed from c5aac2b10a7d12cf80003db5a443e33842ef2c30 to dd9d61452c649d45ade1f3a5ca852566aa6e7e7c

Branch pushed to git repo; I updated commit sha1. New commits:

dd9d614Update copyright and authors for files.

comment:67 Changed 3 years ago by tscrim

Done, and now the patchbot is (morally) green.

comment:68 Changed 3 years ago by git

  • Commit changed from dd9d61452c649d45ade1f3a5ca852566aa6e7e7c to 986df060ee9c0453b670e6e974654580f787d8f3

Branch pushed to git repo; I updated commit sha1. New commits:

986df06Merge branch 'develop' into public/combinat/super_rsk-24894

comment:69 Changed 3 years ago by tscrim

Green patchbot (still).

comment:70 Changed 3 years ago by gh-ChamanAgrawal

  • Status changed from needs_review to positive_review

comment:71 Changed 3 years ago by chapoton

  • Milestone changed from sage-8.9 to sage-9.0

moving milestone to 9.0 (after release of 8.9)

comment:72 Changed 3 years ago by vbraun

  • Branch changed from public/combinat/super_rsk-24894 to 986df060ee9c0453b670e6e974654580f787d8f3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.