George reviews `partition.py` code

[MareoRaft]

George, please review the files in the following order (it's perfectly okay if you don't finish everything):

1. core.py
2. partition.py
3. k_shape.py
4. skew_partition.py
5. root_ideal.py
6. strong_marked_tableau.py
7. all.py

[MareoRaft]

I'm thinking core.py is probably not the right name for that file. I decided that the most appropriate location for files here that aren't additions to existing classes is within the sage/combinat/sf/ folder.

You can see that I've got core.py and partition.py already in an official-sage git-branch over at https://trac.sagemath.org/ticket/25931

UPDATE: migrated ticket: sagemath/sage#25931

[ghseeli]

Okay, here come my coments (for now).

1. core.py

1a) The summand function already exists in Sage for symmetric polynomials. Namely,

sage: Sym = SymmetricFunctions(FractionField(QQ['t']))
sage: s = Sym.s()
sage: elm = s.an_element()
sage: elm.terms()
[3*s[2], 2*s[], 2*s[1]]

1b) The prod function already exists in Sage, although it is implemented differently. Is there something wrong with Sage's implementation?

1c) I worry about this is_k_schur function. It could be misleading. For instance, I could create

sage: Sym = SymmetricFunctions(FractionField(QQ['t']))
sage: ks4 = Sym.kschur(4)
sage: elm = ks4.an_element(); elm
2*ks4[] + 2*ks4[1] + 3*ks4[2]
sage: elm.parent().__class__.__name__
'kSchur_with_category'

is this element truly a k-Schur function? It is a linear combination of them. I honestly do not know the right mathematical answer here. Second concern, though. What is supposed to happen here?

sage: Sym = SymmetricFunctions(FractionField(QQ['t']))
sage: ks4 = Sym.kschur(4)
sage: s = Sym.s()
sage: is_k_schur(s(ks4([4,4,2]))
???

Arguably, a user could think this should be true. After all, a k-schur function in any other basis is still a kschur function. However, I think this would return as False.

1d) I assume the commented out InfiniteDimensionalFreeRing is not going into your ticket? You should probably delete it from the trac branch.

2. partition.py

2a) k_size needs some sort of example / unit test.

2b) Amusing comment in k_boundary should probably go. I also was not quite clear on the fact that k_boundary returns a list of points, not a list of cells.

2c) Line 65 should read "it will be output as an ordered list." The other was is grammatically valid, but a lot less common, imo.

2d) For k_rim, I am not quite sure what the reference text [genocchi] is from the code, although it works in your own documentation build here on github. I also do not quite like calling it "the intersection" of the k-boundary and the k-interior because, if I took the two things as sets of cells and intersected, they would be empty.

2e) has_k_rectangle needs a docstring.

2f) Why is the implementation for is_symmetric not just

def is_symmetric(ptn):
    return ptn == ptn.conjugate()

2g) This next function will need to be renamed, I am pretty sure. Also, "lexicographically" not "lexigraphically."

2h) to_k_core does not work all the time. Consider to_k_core([4],3). No amount of shifting will make this a 3-core, but your code does not raise an error.

[MareoRaft]

George,

This is truly sage advice! ;-) I have addressed most issues and have offered options for others. For the issues where I write "(please reply)", please let me know which option you think it preferable. If you are unsure then we can consult Mike Zabrocki or something. I will change the code in the sage ticket itself after this review process is over.

Best,
Matt

1a) deleted summand
1b) deleted prod

1c) (please reply) is_k_schur was mostly for internal use. Propose changing to __is_k_schur.

1d) removing core.py entirely. Until building symmetric functions with the infinite dim free ring works, the code feels useless.

2a) added

2b) (please reply) I think you meant "boundary", which is a different concept than k_boundary. This is like drawing the shape of the partition and literally tracing its boundary with your pencil, then restricting to integer coordinates.

It exists because it is needed to compute the k_rim. However, I thought it might be useful in its own right.

I could include two hand-drawn pictures in the docs to go along with the two examples. (This brings me to an aside: are images allowed in sage documentation? They certainly work, but could make sage project file-size too large)

2c) sure

2d) (please reply) It works because Sphinx allows references to work across multiple files. Alternatively, I could do one of the three options:

1. For each file, include that file's references at the top of the file
2. For each file, the first time each reference appears in the file, put it at the bottom of that docstring. (convenient for user)
3. I can move the targets of the references to https://doc.sagemath.org/html/en/reference/references/index.html

Sadly, there is not consistency within sage documentation. I have seen all three options when it comes to references. (UPDATE: I have found here that the master reference file is what sage wants: https://doc.sagemath.org/html/en/developer/coding_basics.html)

I could add "If you consider each cell (i, j) geometrically as the square whose vertices are (i, j), (i, j+1), (i+1, j), (i+1, j+1), then the k_rim is the intersection...", or I could instead include a picture.

2e) done

2f) changed. (i had a more efficient algo, but it had a glitch and so i've scrapped it)

2g) changed to next_within_bounds.

2h) now raises a value error if the result is not a k core.

[ghseeli]

General comment: I agree with what you put in issue #8 , so I will stop making comments that pertain to any of those.

1c) Yeah, that is fine by me.

2b) I am not sure about images in documentation, although I agree they would be nice. You could maybe do some ASCII art...

2d) Okay great, as long as we know what the desired method is, we are good to go.

3. k_shape.py

3a) Maybe change all "iff" in documentation to "if and only if." I know most mathematicians probably get it, but might as well just do the find and replace and that way it looks more "professional."

3b) The assert statement if h_bounds may as well have an error message like "Inputs do not form a k shape." In general, probably a nice thing to put on all assert statements, although may not be necessary since the code is pretty clear.

3c) In h_bounds you pad with a 0 row and an infinity row. It might be nice to have a test that shows how that behavior actually shows up.

3d) I assume that the is_irreducible and such have the method=2 for some future extensibility (or maybe there used to be a 1?). Regardless, for putting it into sage, you might as well just get rid of that complication until multiple methods do, in fact, exist.

More on 3 to come later...

[MareoRaft]

Fair warning: a few of the functions in future modules you have not yet reviewed already exist in sage. If I've already found it elsewhere in sage, i put a comment in the source code immediately below the function declaration saying "this ALREADY exists in sage" or similar.

[MareoRaft]

@ghseeli The ticket for partition.py is now ready for non-me people.

UPDATE: migrated ticket: sagemath/sage#25931