Opened 4 years ago
Closed 3 years ago
#26939 closed enhancement (fixed)
Adding Young's raising operators
Reported by:  George H. Seelinger  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  combinatorics  Keywords:  raising operators, symmetric functions 
Cc:  Anne Schilling, Travis Scrimshaw, Matthew Lancellotti, Mike Zabrocki  Merged in:  
Authors:  Matthew Lancellotti, George H. Seelinger  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  befbd71 (Commits, GitHub, GitLab)  Commit:  befbd710a44c7f963337eb9d651af1953967f82c 
Dependencies:  Stopgaps: 
Description
In the course of my work with Jennifer Morse and ghMareoRaft?, we have done some experimenting with raising operators. This code is a cleanedup version of what we have been using and I think it could be useful to other people since these operators come up here and there in combinatorics papers.
Change History (23)
comment:1 Changed 4 years ago by
Branch:  → u/ghseeli/26939implementraisingoperators 

Commit:  → 9dde00f5835425953f42baa26c0eea04dbba765a 
comment:2 Changed 4 years ago by
Commit:  9dde00f5835425953f42baa26c0eea04dbba765a → 377bc9b09ca17710ddf3b545ae23977c683742f5 

comment:3 Changed 4 years ago by
The current code is ready for (probably intensive!) review. A few things I already know will probably need to be addressed, but I would like guidance on.
1) Not everything is PEP8 ! In particular, how does one PEP8 the doctests correctly? Or is it fine asis? This is not explained in the developer's guide.
2) I am still relatively inexperienced developing against the coercion model. I have tried to use the conversion functionality because the relevant morphisms do not meet the axioms of a coercion. I would welcome any constructive feedback so I can learn more!
3) This code structure has evolved over time, so there might be bits and pieces leftover that seem unnecessary. I have tried my best to clean them out, but if you see anything, I am more than happy to reevaluate.
4) Some of this code may seem overly general? However, I already have additions/expansions in mind stemming from some research projects and other papers I have read, but I wanted to start small.
comment:4 Changed 4 years ago by
Cc:  Anne Schilling Travis Scrimshaw Matthew Lancellotti added 

Status:  new → needs_review 
comment:5 Changed 4 years ago by
Commit:  377bc9b09ca17710ddf3b545ae23977c683742f5 → d848672545c3ceda51679857e939764c4502cd8b 

Branch pushed to git repo; I updated commit sha1. New commits:
d848672  Fixing documentation typos in combinat/partition_operator_algebra.py

comment:6 Changed 4 years ago by
Milestone:  sage8.6 → sage8.7 

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sagepending or sagewishlist.
comment:7 Changed 4 years ago by
Milestone:  sage8.7 → sagepending 

Changing milestone to sagepending until further activity.
comment:8 Changed 4 years ago by
A good place to start for selfreviewing and cleaning up is looking at the patchbot output (https://patchbot.sagemath.org/ticket/26939/ , link to it is currently a green check with a red x, indicating it passes all tests, but there are things to be fixed).
In terms of coverage, there are some methods that do not have any doctests, and those should be added.
For python3_py, Sage can currently use Python2, but all code being put in is meant to be compatible with Python3 for an eventual transition. It mentions on specific line that usess something like .iteritems(), which is being deprecated in Python.
Pyflakes lists some variables/imports that are no longer used.
Blocks tries to check to make sure the formatting for doc strings is proper, it looks like there might be something wrong with one INPUT:: string.
Not sure why nonASCII is giving a complaint. I believe the x in startup modules is acceptable.
comment:9 Changed 4 years ago by
Commit:  d848672545c3ceda51679857e939764c4502cd8b → 92c47cecbef304a15ddcb2ba2a99558d63f5d494 

Branch pushed to git repo; I updated commit sha1. New commits:
92c47ce  Various cleanups to partition_shifting_algebras suggested by patchbot.

comment:10 Changed 4 years ago by
Thank you for the feedback! I have tried to address most of the problems, but I still need to document the various __init__
methods. When we wrote the code, I guess we did not adhere to the special Sage conventions around how to document the constructor method itself vs documenting the class. I will have to deal with that part tomorrow.
comment:11 Changed 4 years ago by
Commit:  92c47cecbef304a15ddcb2ba2a99558d63f5d494 → 2f2b870cfc4f73e36862b947d4a8fe7f0d944282 

Branch pushed to git repo; I updated commit sha1. New commits:
2f2b870  Added documentation to partition_shifting_algera

comment:12 Changed 4 years ago by
Hi, Kevin! I have attempted to fix all the issues from the patchbot report, so we will see if it is a happier robot next time it comes around. Thank you again for pointing out the problems!
comment:13 Changed 3 years ago by
Cc:  Mike Zabrocki added 

comment:14 Changed 3 years ago by
Branch:  u/ghseeli/26939implementraisingoperators → public/combinat/partition_shifting_algebra26939 

Commit:  2f2b870cfc4f73e36862b947d4a8fe7f0d944282 → 391ca20cde9e99719c5a5d515f2c7883b352c786 
Milestone:  sagepending → sage8.9 
Reviewers:  → Travis Scrimshaw 
I went through and did a somewhat major rewrite of the structure of the code. Functionally, the main class of ShiftingOperatorAlgebra
works just like before except I removed the ambient space as it was unnecessary as far as I could tell (and not really meant to be used). I removed the RaisingOperatorAlgebra
as specifically defining that subalgebra didn't seem useful as there were no special methods requiring that subalgebra (the ij()
method is welldefined in the ShiftingOperatorAlgebra
). I also did some other cleanup of the code and doc while I was going through things. I will need someone to review my changes, but if mine are good, then it is a positive review.
New commits:
ed3f9c5  Merge branch 'u/ghseeli/26939implementraisingoperators' of git://trac.sagemath.org/sage into u/ghseeli/26939implementraisingoperators

391ca20  Rewriting the partition shifting algebra code.

comment:15 followup: 16 Changed 3 years ago by
Hi, Travis! Thank you for doing this! I agree with your assessments. I will review your changes this weekend. Also, Mike suggested that I be a little more mathematically detailed in the documentation, so I will probably add a little bit to the docstring describing the ShiftingOperatorAlgebra
class.
comment:16 Changed 3 years ago by
Replying to ghseeli:
Hi, Travis! Thank you for doing this! I agree with your assessments. I will review your changes this weekend.
Thank you.
Also, Mike suggested that I be a little more mathematically detailed in the documentation, so I will probably add a little bit to the docstring describing the
ShiftingOperatorAlgebra
class.
Yes, that probably would be good. I tried to do a little bit of this, but I am not at all an expert in this, so I can only do so much.
comment:17 Changed 3 years ago by
Commit:  391ca20cde9e99719c5a5d515f2c7883b352c786 → 0d6f8d07f50cd50da86c4e9b9638b972a1cc44db 

Branch pushed to git repo; I updated commit sha1. New commits:
0d6f8d0  Added some additional documentation and removed duplicate reference

comment:18 Changed 3 years ago by
Okay, Travis! I reviewed your changes and I think they are good! I tried to flesh out the documentation a bit more, too, although it is not perfect. I also removed a duplicate reference so that the documentation would actually build. Anyways, if you are satisfied, I am fine moving this ticket to 'positive_review'. Thank you!
comment:19 Changed 3 years ago by
Can you make these additional changes real quick (otherwise I can do it later today)? Once done (and you agree with them), positive review.
Punctuation and using standard Sage macros:
 `s = x_1^{a_1}x_2^{a_2} \cdots x_r^{a_r} \in S` where `a_i \in  \mathbb{Z}` and `r \geq \ell`, we get that `s.\lambda = (\lambda_1  + a_1, \lambda_2 + a_2,\ldots,\lambda_r+a_r)` where we pad + `s = x_1^{a_1}x_2^{a_2} \cdots x_r^{a_r} \in S`, where `a_i \in + \ZZ` and `r \geq \ell`, we get that `s.\lambda = (\lambda_1 + + a_1, \lambda_2 + a_2,\ldots,\lambda_r+a_r)`, where we pad
Too many examples close together IMO:
 examples of what this looks like with specific bases, see the  examples below. + examples of what this looks like with specific bases, see below.
comment:20 Changed 3 years ago by
Commit:  0d6f8d07f50cd50da86c4e9b9638b972a1cc44db → befbd710a44c7f963337eb9d651af1953967f82c 

Branch pushed to git repo; I updated commit sha1. New commits:
befbd71  Minor documentation changes to partition shifting algebra.

comment:21 Changed 3 years ago by
Status:  needs_review → positive_review 

comment:22 Changed 3 years ago by
Milestone:  sage8.9 → sage9.0 

moving milestone to 9.0 (after release of 8.9)
comment:23 Changed 3 years ago by
Branch:  public/combinat/partition_shifting_algebra26939 → befbd710a44c7f963337eb9d651af1953967f82c 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Added implementation of Young's raising operators.