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

add is_trivial method for groups #36873

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Conversation

mantepse
Copy link
Collaborator

@mantepse mantepse commented Dec 13, 2023

We add a method to check whether a permutation group is trivial, that is, consists only of the identity element.

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have created tests covering the changes.

@mantepse mantepse changed the title add is_trivial method add is_trivial method for permutation groups Dec 13, 2023
Return ``True`` if this group is the trivial group.

A permutation group is trivial, if it consists only of the
identity element, that is, if it has no generators.
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is a very welcome addition, thanks. The code is good as far as I understand. A small complaint about the docstring: In Sage a trivial group may in fact have a (trivial) generator. To my surprise, here we specify no generators but end up having one generator anyway:

sage: PermutationGroup([],domain=[1,2]).gens()
((),)

If the docstring mentions generators (and I think it is a good idea, since it clarifies to the user what the code will actually be checking), it should mention this surprising possibility.

(I believe/hope it is not possible to make a group with several trivial generators, at least not easily. If that is a possibility, we would need to check that all generators are trivial.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, actually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh well, it is possible and not that difficult...

sage: P=PermutationGroup([(),()], canonicalize=False)
sage: P.gens()
((), ())
sage: P.order()
1

A human would likely not create such a group on purpose, but I imagine some code might. Anyway it is possible. Mathematically of course this is a trivial group, so Sage should recognize it as one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

crossed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good now!

@videlec
Copy link
Contributor

videlec commented Dec 13, 2023

Why is this new method restricted to permutation groups and not made available for groups in general? To my mind, such code would better be in the Groups or possibly Monoids category.

@mantepse
Copy link
Collaborator Author

Why is this new method restricted to permutation groups and not made available for groups in general? To my mind, such code would better be in the Groups or possibly Monoids category.

We certainly need finitely generated for this code, although, we can certainly use cardinality for a generic implementation.

It seems that the same code might work well for MatrixGroup, I can add it there, if you want to.

I don't know the category code well enough to add generic code, but if you help me, I'd do it.

@jukkakohonen
Copy link
Contributor

we can certainly use cardinality for a generic implementation.

Interestingly, cardinality can fail in some corner cases, e.g. a special linear group of degree 1:

sage: SL(1, QQ).cardinality()
...
RecursionError: maximum recursion depth exceeded

(Should probably file an issue on this.)

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

we can certainly use cardinality for a generic implementation.

Interestingly, cardinality can fail in some corner cases, e.g. a special linear group of degree 1:

sage: SL(1, QQ).cardinality()
...
RecursionError: maximum recursion depth exceeded

(Should probably file an issue on this.)

yes, this is definitely a bug (it should return NotImplementedError I think). Please file an issue

@jukkakohonen
Copy link
Contributor

Please file an issue

#36876

@mantepse
Copy link
Collaborator Author

I am not sure what to do in the case of finitely presented group. Being trivial is undecidable (https://en.wikipedia.org/wiki/Adian%E2%80%93Rabin_theorem), but of course we can (and should) attempt to decide it. Computing the cardinality seems to be wasteful, however.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

I am not sure what to do in the case of finitely presented group. Being trivial is undecidable (https://en.wikipedia.org/wiki/Adian%E2%80%93Rabin_theorem), but of course we can (and should) attempt to decide it. Computing the cardinality seems to be wasteful, however.

Throw NotImplementedError ?

@mantepse
Copy link
Collaborator Author

I am not sure what to do in the case of finitely presented group. Being trivial is undecidable (https://en.wikipedia.org/wiki/Adian%E2%80%93Rabin_theorem), but of course we can (and should) attempt to decide it. Computing the cardinality seems to be wasteful, however.

Throw NotImplementedError ?

Isn't that worse to checking cardinality?

@mantepse mantepse changed the title add is_trivial method for permutation groups add is_trivial method for groups Dec 13, 2023
@mantepse
Copy link
Collaborator Author

Done, please review.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

It appears that automatic actions triggering is working for you. So it is possible that you not seeing the "Approve and run" button on other PRs is due to your browser/auth, not the setup on GitHub.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

It appears that automatic actions triggering is working for you. So it is possible that you not seeing the "Approve and run" button on other PRs is due to your browser/auth, not the setup on GitHub.

unless someone pushed "Approve and run" button here. It was not me, anyone else?

@dimpase
Copy link
Member

dimpase commented Dec 14, 2023

For solvable groups the world problem is decidable. OK, we can talk about this somewhere later

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 17, 2023
…ationGroup

    
# Fast cardinality method for IntegerVectorsModPermutationGroup

This patch fixes sagemath#36787 by implementing a `cardinality` method for
`IntegerVectorsModPermutationGroup_with_constraints`.  The method
calculates the cardinality using the the [Polya enumeration
theorem](https://en.wikipedia.org/wiki/P%C3%B3lya_enumeration_theorem)
(also known as the cycle index theorem).

It is faster than the the default implementation, which iterates through
the full set to
find the cardinality.

Incidentally this PR fixes also sagemath#36681 so that cardinality and iter no
longer crash in empty-domain situations.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

- sagemath#36873: we can use `is_trivial` from that PR
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36814
Reported by: Jukka Kohonen
Reviewer(s): Dima Pasechnik, Jukka Kohonen, Martin Rubey, Travis Scrimshaw
@mantepse mantepse closed this Dec 18, 2023
@mantepse mantepse deleted the perm_gps/is_trivial branch December 18, 2023 12:28
@mantepse mantepse restored the perm_gps/is_trivial branch December 18, 2023 12:28
@mantepse mantepse reopened this Dec 18, 2023
@mantepse
Copy link
Collaborator Author

sorry, I pressed the wrong button

Copy link

Documentation preview for this PR (built with commit 1ef1380; changes) is ready! 🎉

@vbraun vbraun merged commit 2343c20 into sagemath:develop Dec 19, 2023
33 of 35 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 19, 2023
@mantepse mantepse deleted the perm_gps/is_trivial branch January 27, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants