sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Adding Young's raising operators #26939

Closed ghseeli closed 5 years ago

ghseeli commented 5 years ago

In the course of my work with Jennifer Morse and gh-MareoRaft, we have done some experimenting with raising operators. This code is a cleaned-up 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.

CC: @anneschilling @tscrim @MareoRaft @zabrocki

Component: combinatorics

Keywords: raising operators, symmetric functions

Author: Matthew Lancellotti, George H. Seelinger

Branch/Commit: befbd71

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/26939

ghseeli commented 5 years ago

Commit: 9dde00f

ghseeli commented 5 years ago

Branch: u/ghseeli/26939-implement-raising-operators

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

377bc9bAdded implementation of Young's raising operators.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 9dde00f to 377bc9b

ghseeli commented 5 years ago
comment:3

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 as-is? 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 re-evaluate.

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

d848672Fixing documentation typos in combinat/partition_operator_algebra.py
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 377bc9b to d848672

embray commented 5 years ago
comment:6

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 sage-pending or sage-wishlist.

ghseeli commented 5 years ago
comment:7

Changing milestone to sage-pending until further activity.

1adc0eef-8957-46d9-975b-2dd71dfbd9ba commented 5 years ago
comment:8

A good place to start for self-reviewing 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 non-ASCII is giving a complaint. I believe the x in startup modules is acceptable.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

92c47ceVarious cleanups to partition_shifting_algebras suggested by patchbot.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from d848672 to 92c47ce

ghseeli commented 5 years ago
comment:10

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

2f2b870Added documentation to partition_shifting_algera
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 92c47ce to 2f2b870

ghseeli commented 5 years ago
comment:12

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!

tscrim commented 5 years ago

Changed branch from u/ghseeli/26939-implement-raising-operators to public/combinat/partition_shifting_algebra-26939

tscrim commented 5 years ago

Changed commit from 2f2b870 to 391ca20

tscrim commented 5 years ago
comment:14

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 well-defined 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:

ed3f9c5Merge branch 'u/ghseeli/26939-implement-raising-operators' of git://trac.sagemath.org/sage into u/ghseeli/26939-implement-raising-operators
391ca20Rewriting the partition shifting algebra code.
tscrim commented 5 years ago

Reviewer: Travis Scrimshaw

ghseeli commented 5 years ago
comment:15

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.

tscrim commented 5 years ago
comment:16

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 391ca20 to 0d6f8d0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

0d6f8d0Added some additional documentation and removed duplicate reference
ghseeli commented 5 years ago
comment:18

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!

tscrim commented 5 years ago
comment:19

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.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 0d6f8d0 to befbd71

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

befbd71Minor documentation changes to partition shifting algebra.
fchapoton commented 5 years ago
comment:22

moving milestone to 9.0 (after release of 8.9)

vbraun commented 5 years ago

Changed branch from public/combinat/partition_shifting_algebra-26939 to befbd71