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

random_element for colored and signed permutations #34925

Closed
mantepse opened this issue Jan 20, 2023 · 7 comments · Fixed by #36155
Closed

random_element for colored and signed permutations #34925

mantepse opened this issue Jan 20, 2023 · 7 comments · Fixed by #36155

Comments

@mantepse
Copy link
Collaborator

Before this ticket,

sage: SignedPermutations(10).random_element()

takes forever.

Component: combinatorics

Author: Martin Rubey

Branch/Commit: u/mantepse/random_element_for_colored_and_signed_permutations @ 86d9ac7

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

@mantepse mantepse added this to the sage-9.8 milestone Jan 20, 2023
@mantepse
Copy link
Collaborator Author

@mantepse
Copy link
Collaborator Author

Author: Martin Rubey

@mantepse
Copy link
Collaborator Author

Commit: 86d9ac7

@mantepse

This comment has been minimized.

@mantepse
Copy link
Collaborator Author

New commits:

86d9ac7random_element for colored and signed permutations

@mantepse
Copy link
Collaborator Author

comment:3

There is a design decision for SignedPermutations I do not understand: it inherits from ColoredPermutations, but the colors are {+1, -1} instead of IntegerModRing(2).

In fact, also IntegerModRing looks strange to me, because we really only consider the colors as a group, not a ring.

It seems to me that the only reason for this is the result of SignedPermutation.colors(). However, this choice requires us to implement very many methods twice.

I see a few possibilities:

  1. keep the decision and ignore its downsides
  2. introduce a group containing two elements {1, -1}.
  3. only override SignedPermutation.colors().
  4. change the colors to {0, 1}.

@tscrim
Copy link
Collaborator

tscrim commented Feb 19, 2023

The issue for this ticket (mostly just for the official record) comes from the fact it is using the default random_element() that lists all the elements and pulls a random element from that.

At some point, we probably should also implement an unrank() and __getitem__ method since that is also easy. I just unrolled the unrank() for the Cartesian product.

def unrank(n):
    k = self._m * self._n
    N = n // k
    perm = self._P.unrank(N)
    colors = [None] * self._n
    C = n % k
    for i in range(self._n-1,-1,-1):
        colors[i] = self._C.unrank(C % self._m)
        C //= self._m
    return self.element_class(self, tuple(colors), perm) 

Some comments about the design:

Since IntegerModRing is a ring, it is also naturally a group (under +). Plus it is fast.

The colors themselves don't need to be an actual elements of a group parent since they are used internally. If it helps, you can think of them as elements in a facade parent. It is more natural for the type B Weyl/Coxeter group combinatorics people to work with {1, -1} as the "colors", and there isn't too much extra that needs to be most-duplicated IIRC. If this ends up scaling to a bigger problem, then we can revisit the design and see if we can remove the duplication.

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 1, 2023
    
this fixes sagemath#34925

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#36155
Reported by: Frédéric Chapoton
Reviewer(s): Travis Scrimshaw
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants