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

improvements for cosets #4302

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ThomasBreuer
Copy link
Member

  • access defining data via functions not fields
  • rename acting_domain to acting_group
  • improve documentation
  • in for left/right cosets now delegates to a membership test in a group
  • iterator for left/right cosets now uses a group iterator

addresses #4283 and #4289

- access defining data via functions not fields
- rename `acting_domain` to `acting_group`
- improve documentation
- `in` for left/right cosets now delegates to a membership test in a group
- iterator for left/right cosets now uses a group iterator
@ThomasBreuer ThomasBreuer added enhancement New feature or request topic: groups labels Nov 12, 2024
@fingolfin
Copy link
Member

@ThomasBreuer does it resolve those PRs completely? Then you can write resolves #X and resolves #Y to make sure merging this PR automatically closes those issues. Alternatively you can manually link them via the web view
Screen Shot 2024-11-13 at 08 10 38

@ThomasBreuer
Copy link
Member Author

does it resolve those PRs completely

No.

@@ -147,3 +147,6 @@ Base.@deprecate_binding in_linear_system is_in_linear_system
@deprecate minimal_generating_set(G::GAPGroup) minimal_size_generating_set(G)
@deprecate has_minimal_generating_set(G::GAPGroup) has_minimal_size_generating_set(G)
@deprecate set_minimal_generating_set(G::GAPGroup, v) set_minimal_size_generating_set(G, v)

@deprecate acting_domain(C::GroupCoset) acting_group(C)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@deprecate acting_domain(C::GroupCoset) acting_group(C)
# deprecated for 1.3
@deprecate acting_domain(C::GroupCoset) acting_group(C)

just so we keep this information for the future

Add the type of the *subgroup* as a parameter,
then we can prescribe a better `Base.IteratorSize(::Type{<:GroupCoset})`,
as proposed in oscar-system#4289.

Note that in principle, we could omit the type of the *big group* from the
parameters since it is the `parent_type` of the element type parameter.
(The design of the `GroupCoset` type dates back to the times when
we thought that a group has the same type as its subgroups.
At the time when this idea was given up, I should have changed
`GroupCoset` to take the *subgroup* type as a parameter.)
This was tricky:
The error messages were misleading, the problem occurred on a lower level.
@ThomasBreuer
Copy link
Member Author

Run tests / test (1.6, short, ubuntu-latest) failed with a segmentation fault, restarted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic: groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants