changes to partition for k combinat

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

The first changes to sage in order to migrate the https://github.com/MareoRaft/k_combinat_for_sage library (see also #25295) involve changing partition.py.

This ticket is now READY FOR REVIEW :)

CC: @ghseeli

Component: combinatorics

Keywords: partition, k, combinat

Author: Matthew Lancellotti

Branch/Commit: 46fa747

Reviewer: George H. Seelinger

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

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

Branch: public/k_combinat/partition/25931

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

New commits:

3a3e510 first commit. testing.

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 The first changes to sage in order to migrate the https://github.com/MareoRaft/k_combinat_for_sage library (see also #25295) involve changing `partition.py`.

-More soon
+This ticket is in-progress and *not* ready for review yet.  More soon.

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

Commit: 3a3e510

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

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

a88e3c6 test 2. non code change.

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

Changed commit from 3a3e510 to a88e3c6

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

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

`d2f9d8a`	`unit tests for partition passing. next need to run sage tests.`
`9849b13`	`sage tests passing`

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

Changed commit from a88e3c6 to 9849b13

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

Changed commit from 9849b13 to ca2f091

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

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

`3b71f13`	`newest docstrings, newest changed to k_combinat_for_sage, and tests added`
`6b3987c`	`simply moved all the methods as one giant chunk to a more appropriate place. the git diff looks really complicated though because git can be 'stupid' at times.`
`ca2f091`	`references`

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 The first changes to sage in order to migrate the https://github.com/MareoRaft/k_combinat_for_sage library (see also #25295) involve changing `partition.py`.

-This ticket is in-progress and *not* ready for review yet.  More soon.
+This ticket is now READY FOR REVIEW :)

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

comment:5

This branch is now ready for review! George Seelinger has already reviewed this code (https://github.com/MareoRaft/k_combinat_for_sage/issues/6) and the code has been modified accordingly, so there shouldn't be much left to do.

One small question:

The sagedocs say I should use "this" terminology in my docstrings for methods. For example, given a method score for the Partition object, the docstring would say something like "find the score of this partition" as opposed to "given a partition, find its score". Should I go ahead and alter all of the language in my method docstrings accordingly? (It's a lot of work, so I want to make sure it's necessary before I spend a lot of time doing it).

tscrim commented 6 years ago

comment:6

Replying to @MareoRaft:

This branch is now ready for review! George Seelinger has already reviewed this code (https://github.com/MareoRaft/k_combinat_for_sage/issues/6) and the code has been modified accordingly, so there shouldn't be much left to do.

One small question:

The sagedocs say I should use "this" terminology in my docstrings for methods. For example, given a method score for the Partition object, the docstring would say something like "find the score of this partition" as opposed to "given a partition, find its score". Should I go ahead and alter all of the language in my method docstrings accordingly? (It's a lot of work, so I want to make sure it's necessary before I spend a lot of time doing it).

I am a very strong -1 to that (grr...they snuck that change in). Use ``self`` as that never causes any problems or ambiguity. Also, for methods, the first argument must be self.

However, your docstrings are mal-formatted. It should be:

def foo(self, bar):
    r"""
    One line description saying what the method does.

    Long description but keep each line at most 80 characters
    unless necessary. This is where you put the more conceptual
    information.
    """

You might find the .. PLOT:: syntax useful. However, just use the ascii_art or .pp() for the diagrams of the partitions instead of pictures. That is sufficient, but I think is generally overkill for such small examples.

The _is_sequence is mostly useless and does not test what its name suggests (e.g., it does not handle tuple). It also is not used. Remove it.

Similarly for is_*_decreasing. Just inline them. The code makes it clear what is going on.

You are too verbose in your descriptions in, e.g., boundary() as you are just saying the same thing as the output. (That TESTS:: block is mal-formatted as well.)

Error messages are not complete sentences, so not capitalized and no period at the end.

Typo: THerefore.

It is better to use _Partitions(la) than Partition(la) as there is less indirection.

I will likely have more comments once these things are fixed.

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

comment:7

Thanks for the rapid reply!

I'm putting the 80 char docstrings thing on my to-do list. I've made the requested change in my latest commit, with a few exceptions / need for further discussion.

_is_sequence is used in root_ideal.py and all.py (see https://github.com/MareoRaft/k_combinat_for_sage/search?q=_is_sequence&unscoped_q=_is_sequence). Remember that the ultimate goal is to migrate the majority of the k_combinat_for_sage project over to sage. There are situations where both :class:Compositions and :class:Partitions (and possibly lists) are acceptable inputs. This is what necessitated the _is_sequence utility function. The function could be renamed. I feel it does need to exist somewhere though. I suppose it could be moved to root_ideal.py.

The boundary explanation is admittedly confusing. I can try to rewrite it. What are you referring to as "the output"?

Is it a requirement that error messages are not sentences? I could not find it in the conventions (https://doc.sagemath.org/html/en/developer/coding_basics.html). I see other files within sagemath where error messages are full sentences (such as stream_cipher.py). Moreover, I don't think good grammar is a bad thing.

Thanks again for your input. Have a great day.

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

Changed commit from ca2f091 to bbdd795

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

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

`c46cc6d`	`newline at beginning of docstrings. _Partitions instead of Partition.`
`bbdd795`	`make first param of every method 'self' and update docstrings accordingly`

tscrim commented 6 years ago

comment:9

Replying to @MareoRaft:

_is_sequence is used in root_ideal.py and all.py (see https://github.com/MareoRaft/k_combinat_for_sage/search?q=_is_sequence&unscoped_q=_is_sequence). Remember that the ultimate goal is to migrate the majority of the k_combinat_for_sage project over to sage. There are situations where both :class:Compositions and :class:Partitions (and possibly lists) are acceptable inputs. This is what necessitated the _is_sequence utility function. The function could be renamed. I feel it does need to exist somewhere though. I suppose it could be moved to root_ideal.py.

It is still a mostly useless function by itself and better replaced by the code directly. -1 on the function even existing.

The boundary explanation is admittedly confusing. I can try to rewrite it. What are you referring to as "the output"?

        has boundary [(1, 0), (1, 1), (0, 1)]::

            sage: Partition([1]).boundary()
            [(1, 0), (1, 1), (0, 1)]

I mean the output of the function call. For example, here is needless repetition and text.

Is it a requirement that error messages are not sentences? I could not find it in the conventions (https://doc.sagemath.org/html/en/developer/coding_basics.html). I see other files within sagemath where error messages are full sentences (such as stream_cipher.py). Moreover, I don't think good grammar is a bad thing.

This is a Python convention, and we are trying to adhere to it. Sage is not uniform, but you should not make the situation worse.

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

comment:10

It is still a mostly useless function by itself and better replaced by the code directly.

When you say "useless by itself", do you mean useless to end users? Certainly, it is not uncommon to have code that is not meant to be used by end users. These are components which help other parts of the code to run. Many languages designate pieces of code as "private" or "protected" for this reason.

The benefit of a utility function like this one is eliminating redundancy. This makes code shorter. If a change needs to be made to the code, it only needs to be made once, instead of in many places simultaneously. It also decreases the chances of accidentally changing one thing and not another, causing glitches and loss of time troubleshooting. In addition, it helps keep behavior/usage consistent across many end-user functions.

The benefit of not having such a function is that other functions become more self-contained, which makes it easier for somebody to alter one of those functions in the future.

I wish to learn and understand why certain choices are made. I think you should elaborate on the "why" behind your opinions, so that the sage community can learn and have lasting change where improvements need to be made, and disabuse misconceptions when they occur.

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

Changed commit from bbdd795 to 03e3ed5

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

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

03e3ed5 change error messages from sentences to fragments

tscrim commented 6 years ago

comment:12

It is a short 1-liner and requires people to look at your function to determine what you consider a sequence. As you said, having it makes it less self-contained, which in this case is actually less verbose. There is very little to no redundancy being removed with such a function. Find and replace does a great job. Also note that function calls in Python are never inlined and do incur a speed penalty. I completely understand why we write short helper functions, but Python is not C, Java, etc.

Also, please avoid such grandiose statements such as those in your last paragraph. It comes off as being really snide to me.

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

Changed commit from 03e3ed5 to 1911569

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

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

`a33d73e`	`remove _is_sequence, is_*_decreasing, k dim list functions`
`1911569`	`inline k rect dim list function, remove newlines`

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

comment:14

@tscrim Do you have any more comments / requested changes?

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 6 years ago

comment:16

Okay, great. I'm marking this review as complete, but please feel free to change the status back and bring up any issues if they arise.

fchapoton commented 6 years ago

comment:17

reviewer name, please

tscrim commented 6 years ago

comment:18

You do not get to mark a positive review just because there are no further comments. I have not had time to look at this further.

fchapoton commented 5 years ago

comment:20

branch is red

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 5 years ago

comment:21

I originally did not put a reviewer name because I didn't know whether I should put George, Travis, or someone else. After some thought and discussion, George has agreed to be the reviewer. I think George is the most appropriate choice for a few reasons.

George is the most familiar with this code. I discussed various issues with George in-person while writing this code. George is now using this code to help him with his own research. As a user of the code, George can see if/when it has any issues. This code was originally written for Jennifer Morse for the purposes of certain research topics. As Jennifer Morse's student who is working in and among those same topics, George will be working with this code long-term and it is in his own best interest to make sure it is good. Also, George has worked professionally as a data scientist and is an exceptional programmer. He holds code to very high standards. George has conducted the majority of the review so far, so it makes sense for him to finish off what he has already started.

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 5 years ago

Reviewer: George Seelinger

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

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

f3dc7bb resolve conflict in the references index.rst file

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

Changed commit from 1911569 to f3dc7bb

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

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

ef5701c eliminate 2 images, keep 1 image, fix indentation in docstring

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

Changed commit from f3dc7bb to ef5701c

ghseeli commented 5 years ago

Changed reviewer from George Seelinger to George H. Seelinger

ghseeli commented 5 years ago

comment:27

a) I am going to recommend removing all papers not currently being referenced in the added documentation. These are currently BMPS2018b, Fuchs1994, Fun2012, HS1968, LN2014. If future tickets need them, they can be added in at that time.

b) Documentation for k_size is redundant: the NOTE and the second sentence are the same.

c) The TESTS block of boundary does not adhere to Sage conventions.

d) For the second sentence of the boundary documentation, I think it might be good to specify French convention when mentioning the northeasternmost point. Maybe for clarity, a description for both French and English conventions should be provided (so southeasternmost point in English convention), especially considering English convention is the default for Sage, although French is used in the cited paper for k_boundary.

e) I think has_rectangle should have an INPUT block in its documentation to specify these inputs are supposed to be integers. Also, I suggest implementation to the effect of return self.to_exp(w)[w] >= h since the .to_exp() function already does the counting of row lengths.

f) Rephrase first sentence of has_rectangle and has_k_rectangle to meet Sage standards: "Return True if ..."

g) has_k_rectangle EXAMPLES are not obviously has_k_rectangle tests. They should be commented with # indirect doctest and maybe there should be one example that highlights the difference between this method and is_k_reducible (if it is_k_reducible is kept; see item l below).

h) is_k_bounded docstring should begin "Return True" instead of "Returns True"

i) Nitpicking on is_k_bounded, partitions are ordered such that the first entry is the largest. So, I would recommend something like

if self.is_empty():
    return True
else:
    return self[0] <= k

j) Documentation of is_k_reducible, is_k_irreducible, has_rectangle, has_k_rectangle should all say "its Ferrer's diagram" not "it's Ferrer's diagram"

k) I think the assertion statement of is_k_reducible should include the comment above in the error message.

l) This is item is just my opinion: it is a little unnecessary to include both is_k_reducible and is_k_irreducible since they are just logical opposites of each other and it adds many lines of redundant documentation (thus extra documentation to maintain). I am not sure whether Sage has a convention about such things or not; indeed, the Permutations class has both an is_reducible and is_irreducible method, but polynomials only have an is_irreducible method. However, I would recommend sticking with just one method; personally, I prefer having is_irreducible methods, like in the polynomial case. Ultimately, I leave this decision up to you.

m) With validating the inputs on next_within_bounds, I might prefer to take the "Garbage in Garbage out" approach and remove the assertions. In particular, I don't see why it would be a problem a priori if a user passed in a tuple, a composition that happens to also be a partition, a core, etc. Also, later in the code, min and max are casted to the appropriate types anyways, which I believe would serve as a good enough input validation.

n) On next_within_bounds, I feel like type is not a good argument name because it is a reserved word in Python. Maybe it should change to partition_type or something of the sort.

o) For is_k_core, there is already an is_core(k) method in the Partition class that does this, so I think is_k_core should probably be removed.

p) For to_k_core, I think there should be a more verbose name so as not to confuse with the to_core(k) method. Also, it is noted in the documentation how it will behave when given a k-1 bounded partition, but I think it might be helpful to make it more explicit. In particular, if part is k-1 bounded, part.to_k_core(k) should be the same as Partition(part.to_core(k-1)). So, it would also make sense to include :meth:`to_core` in a SEEALSO block. Furthermore, it should include a test that triggers the ValueError. An easy example is Partition([3]).to_k_core(2). Finally, I am worried about some of the comments in this method; there are suggested improvements in the method that should just as well be programmed. If this will slow down finishing this ticket, maybe it would be easier to add this method in as a separate ticket.

z) We should PEP 8 the docstrings before final acceptance!

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

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

`6fb0814`	`remove redundant NOTE in docstring`
`9e30751`	`remove citations that haven't been used yet`
`66cadcb`	`fix TEST block formatting. attempt to make boundary explanation more clear.`
`4ebb523`	`address review (a) through (l)`
`5e8f38d`	`address review items (m) through (o)`

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

Changed commit from ef5701c to 5e8f38d

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 5 years ago

comment:29

a) removed: BMPS2018b, Fun2012, LN2014.

did not remove: Fuchs1994, HS1968. These two are part of the develop branch. They appear in the diff because they caused a merge conflict.

b) removed NOTE

c) fixed

d) I have specified the French convention and provided a small example of the French convention. (but i didn't see the need for talking about the english convention because it thought it would be too much) Please read it and let me know if it is satisfactory or not.

Also, should perhaps cite the paper in boundary docstring too?

e1) INPUT block added.

e2) done

f) done

g) something was wrong here. the examples were using is_k_reducible instead of has_k_rectangle. i have changed them to has_k_rectangle.

h) done

i) Took your advice, but now added assert k >= 0

j) done

k) done

l) Definitely a hair splitter. I will leave both.

m) okay, you do have a point. done.

n) So is min and max. I really broke all the rules on this one. But I chose these keywords because they were clear, short, and convenient for the user.

changed type to partition_type (because i think its more clear)

o) Removed, but combined the docstrings since the other one had some issues.

I have :meth:Core in the docstring but i think it's a typo. I'm pretty sure it should be :class:Core?

p) Also, a reference to the "well-known" bijection would be good. Do you know of any (published) sources we can reference?

Do you think this method is redundant?

z) Wow, I can't believe we went through the whole alphabet. I agree, but let's wait until after everything else is resolved.

fchapoton commented 5 years ago

comment:30

you should break lines at 80 characters

ghseeli commented 5 years ago

comment:31

a) Okay, great. I am sorry for jumping the gun. I should have checked my diff against the current develop branch instead of the original one you committed to.

d) I don't think it hurts to add extra citations for the curious user, but it is also not necessary. Up to you.

n) I think using min and max like you have is fine. I believe there is precedent in Sage, too.

o) The usage of :meth:core here is correct, although I can see why it is confusing. There is a notion of taking a "p-core" and a "p-quotient" of a partition as operations, so what the documentation is saying is that if you take the "p-core" of a partition and get the partition back as the output, then the partition was a p-Core to begin with.

p) I think the best reference is the "k-Schur functions and affine Schubert calculus" book by Lam et al. Proposition 1.3. The book is already in the Sage references as [LLMSSZ2013] to make life easier.

I will say that I personally do not see the mathematical use-case for the to_k_core method in this ticket. It is slightly more general than to_core(k) because it will take partitions that are not k-1 bounded and attempt to make them into a k-core, but every usage I have seen of such an operation (shifting rows to make cores) has been done on k-1-bounded partitions. However, there could easily be something I am not aware of.

I was trying to think of how to describe the entire class of partitions that this method would be guaranteed to not fail on, but I do not have an easy answer. That being said, the method is still mathematically valid; I just worry about users getting confused about the difference between to_core(k) and to_k_core, as well as the fact that to_k_core can fail, but such failures are not easy to describe.

z) Yeah, it is amazing how much of the alphabet you can get through when you skip a bunch of letters.

you should break lines at 80 characters

chapoton, thanks for the input. That is what I meant by my item z), but I should have been more explicit!

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 5 years ago

comment:32

d) Decided not to cite because there is no definition of "boundary" in the papers as is used here. The boundary here is useful in calculating the k_rim, and is sensible as its own concept.

o) Are you positive it is correct? It is currently :meth:Core, not :meth:core.

p) to_k_core now deleted because it is NOT a generalization of the canonical bijection as we originally thought

z) My text editor has a feature where you put down a ruler (staff) at 80 characters and then you say "YOU SHALL NOT PASS!"

That's done now.

NOTE: Before your review is complete, can you rerun the doctests? There is an issue with my sage installation and also I haven't had a chance to upgrade to the newer version.

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

Changed commit from 5e8f38d to 5078c75

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

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

`0a9dedf`	`fix comments on to_k_core`
`cd14216`	`remove to_k_core, as it is NOT a generalization of the canonical bijection`
`5078c75`	`restrict docstrings to 80 char width`

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

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

46fa747 Fixed minor documentation issues in combinat.partition.

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

Changed commit from 5078c75 to 46fa747

ghseeli commented 5 years ago

comment:35

o) In the docstring paragraph, :meth:`core` is used. In the SEEALSO block, it honestly makes sense to have both :meth:`core` and :class:`Core`.

Doctests are currently passing for me.

However, documentation is not building because

q) the path for k-rim.JPG is wrong. It should be ../../media/k-rim.JPG.

r) Also, the INPUT blocks were not quite "line broken" correctly.

s) I think describing self in the input block is unnecessary and not typically done in Sage conventions unless there is something out of the ordinary going on.

I have gone ahead and fixed these minor things myself. If you agree with the changes, you may set the ticket to "Positive Review".

9bfcb69a-807d-4b9a-b64e-aba029db57fa commented 5 years ago

comment:36

Thanks kindly for your comprehensive review.

embray commented 5 years ago

comment:37

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.

vbraun commented 5 years ago

Changed branch from public/k_combinat/partition/25931 to 46fa747

sagemath / sage

changes to partition for k combinat #25931