Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Permutations package #3393

Open
wants to merge 16 commits into
base: development
Choose a base branch
from

Conversation

TheGrateSalmon
Copy link

@TheGrateSalmon TheGrateSalmon commented Aug 7, 2024

New package implementing a Permutation type by Sean Grate. Its intent is to have a standard and consistent type for permutations as more combinatorics-focused packages get added to Macaulay2. Much of the inspiration for the implemented methods came from the Sage implementation. This also resolves Issue #2581.

@mahrud
Copy link
Member

mahrud commented Aug 7, 2024

  1. Add your package to the =distributed-packages file, otherwise it's basically ignored.
  2. I think having both Permutations.m2 and Permutations/Permutations.m2 could lead to hard to decipher errors (e.g. try needsPackage "Permutations" inside the Permutations directory). My suggestion would be to put most of the content of Permutations/Permutations.m2 in Permutations.m2, and name the other two files tests.m2 and docs.m2.
  3. Typos:
M2/Macaulay2/packages/Permutations/PermutationsDOC.m2:241: postive ==> positive
M2/Macaulay2/packages/Permutations/PermutationsDOC.m2:622: simulataneously ==> simultaneously

@TheGrateSalmon
Copy link
Author

I should also mention that this package includes code for pattern avoidance and is taken directly from the MatrixSchubert package. The idea is that since pattern avoidance is inherent to permutations, it'll go into Permutations, and following the adoption of this package, the MatrixSchubert package might be overhauled to use the Permutation type from this package for consistency.

-- avoidsPatterns
doc ///
Key
avoidsPatterns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under SeeAlso, you can link to "MatrixSchubert::avoidsAllPatterns" and "MatrixSchubert::isPatternAvoiding".

new Permutation from VisibleList := (typeofPermutation,w) -> w

permutation = method()
permutation List := Permutation => w -> new Permutation from w
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, I think it would be reasonable to move permutations, inversePermutation, and (possibly?) uniquePermutations into this package (along with their tests and documentation), and comment that they were moved from Core. This may break some tests in other packages, in which case we will add this package as their dependency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this make sense and I'll look into it.

Comment on lines +53 to +54
to1Index := w -> (w / (i -> i+1))
to0Index := w -> (w / (i -> i-1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but I'm curious if there was a problem with my suggestion about prepending 0 to switch to 0-index: #2581 (comment)

Copy link
Author

@TheGrateSalmon TheGrateSalmon Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problems, just those helper functions seemed like the most obvious thing to do at the time I had written the bulk of this code (December 2023).

@pzinn
Copy link
Contributor

pzinn commented Aug 8, 2024

One incompatibility with the built-in permutations is that they start at 0, not 1.

@mahrud
Copy link
Member

mahrud commented Aug 8, 2024

One incompatibility with the built-in permutations is that they start at 0, not 1.

Yes, although MatrixSchubert is also 1-indexed. I realize I'm not the main audience of this, so I don't have a strong opinion. Would it be much harder to check whether a permutation is 0 or 1 indexed in every algorithm? Or maybe have easy to use functions for going forth and back?

@TheGrateSalmon
Copy link
Author

One incompatibility with the built-in permutations is that they start at 0, not 1.

Yes, although MatrixSchubert is also 1-indexed. I realize I'm not the main audience of this, so I don't have a strong opinion. Would it be much harder to check whether a permutation is 0 or 1 indexed in every algorithm? Or maybe have easy to use functions for going forth and back?

I think a good solution would be some sort of configuration option when loading the package. So, it defaults to $1$-indexing, but the user can switch to $0$-indexing if they prefer. Something like needsPackage("Permutations", ZeroIndex=>true).

@mahrud
Copy link
Member

mahrud commented Oct 7, 2024

Recently I was using permutations (though not through this package) and I thought of a few wish list items, if you have time to implement them:

  1. VisibleList_Permutation (and MutableList), Matrix_Permutation and Matrix^Permutation (and MutableMatrix) for permuting elements of lists and columns or rows of matrices.
  2. A comparison operator Permutation ? Permutation based on the Bruhat order (is this available elsewhere in M2?)
  3. A method to get the poset of Bruhat order and the Hasse diagram of S_n (including maybe a way to get the subposet below or above a given permutation)
  4. matrix Permutation to get the permutation matrix (I know permToMatrix in MatrixSchubert does this, but given a Permutation type you could just define matrix on it)

I'm happy to merge this with the 1-based notation if at least item 1 above is implemented.

@mahrud mahrud added this to the version 1.24.11 milestone Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants