Skip to content

Add new experimental package ModularGroup for working with finite-index subgroups of SL_2(Z) - #6073

Open
Sebas777-gif wants to merge 34 commits into
oscar-system:masterfrom
AG-Weitze-Schmithusen:modular_group
Open

Add new experimental package ModularGroup for working with finite-index subgroups of SL_2(Z)#6073
Sebas777-gif wants to merge 34 commits into
oscar-system:masterfrom
AG-Weitze-Schmithusen:modular_group

Conversation

@Sebas777-gif

Copy link
Copy Markdown

These are some first basic functions for an OSCAR version of the ModularGroup GAP package. Some of these functions are used in the experimental Origami module, for which there is also an ongoing pull request #5345. The long-term goal is to replace most currently used GAP wrappers with native re-implementations.

Since Origami depends on ModularGroup, but not the other way around, I created this separate branch for ModularGroup. There is still a lot to do, especially writing tests and documentation, but some first feedback from @fingolfin (or others) would be most appreciated. Thanks!

@lgoettgens lgoettgens added the experimental Only changes experimental parts of the code label Jun 18, 2026
@Sebas777-gif
Sebas777-gif marked this pull request as ready for review June 19, 2026 09:03
@fingolfin fingolfin added the enhancement New feature or request label Jun 19, 2026

@lgoettgens lgoettgens left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems I forgot to submit my comments. Sorry for the delay

Comment thread experimental/ModularGroup/docs/doc.main Outdated
Comment thread experimental/ModularGroup/src/exports.jl Outdated
Comment thread experimental/ModularGroup/src/ModularGroup.jl
Comment thread experimental/ModularGroup/src/ModularGroup.jl
Comment thread experimental/ModularGroup/src/ModularGroup.jl Outdated
Comment thread experimental/ModularGroup/src/ModularGroup.jl Outdated
Comment thread experimental/ModularGroup/src/ModularGroup.jl Outdated
Comment thread experimental/ModularGroup/src/types.jl Outdated
Comment thread experimental/ModularGroup/test/runtests.jl Outdated
export r_right_action
export s_right_action
export s_t_decomposition
export t_right_action

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, a bunch of these names do not sound like they should be available to all OSCAR users. But as I am not used to the mathematics involved and its conventions, I leave that for @fingolfin to comment on.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I talked to @wsinuds and she said that at least in the GAP version of the package, all these functions were indeed meant to be available for end users. Maybe there are some different standards for this question in OSCAR though?

@Sebas777-gif
Sebas777-gif requested a review from lgoettgens June 27, 2026 20:28
@fingolfin fingolfin changed the title Experimental package ModularGroup Add new experimental package ModularGroup for working with finite-index subgroups of SL_2(Z) Jul 8, 2026
@fingolfin fingolfin added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Jul 8, 2026

@ThomasBreuer ThomasBreuer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good.
I have added a few comments.

One more thought:
In the membership tests, the code computes the permutation induced by the given word, and then checks whether this permutation fixes 1. Wouldn't it be cheaper to map 1 by the sequence of the generating permutations given by the word, and to check whether the result is 1? This would avoid computing many products of permutations.

SL2Z, _, _ = _SL2Z_fp()
phi = hom(SL2Z, M, [MS, MT])

return [matrix(phi(w)) for w in w_gens]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In each call of this function, the objects MatS, MatT, M, and phi are created anew. They are needed only once. Instead of caching the domain of phi, one could simply cache phi.

Is it really intended to return matrices not matrix group elements?
(The conceptual question behind that is what a ModularGroup object actually is:
Is it a group, and if yes, is it a matrix group?
The stored data are a finitely presented group and an isomorphism from this group to the matrix group SL(2, Z) and an epimorphism $\pi$ to a permutation group, all defined on generators for which the defining relations hold; the ModularGroup object describes the subgroup of SL(2,Z) (or of the finitely presented group) that is given by the preimage of the stabilizer of 1 under $\pi$.)

@Sebas777-gif Sebas777-gif Aug 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, that would probably be better. However, I am wondering if it is advisable to still cache the domain independently. I.e., keep the function _SL2Z_fp(), and then write

const _MATRIX_HOM_CACHE = Ref{GAPGroupHomomorphism{FPGroup, MatGroup{ZZRingElem, ZZMatrix}}}()

function _matrix_hom()
  if !isassigned(_MATRIX_HOM_CACHE)
    MatS = matrix(ZZ, [0 -1; 1 0])
    MatT = matrix(ZZ, [1  1; 0 1])

    M = matrix_group([MatS, MatT])
    MS, MT = gens(M)

    SL2Z, _, _ = _SL2Z_fp()
    _MATRIX_HOM_CACHE[] = hom(SL2Z, M, [MS, MT])
  end

  return _MATRIX_HOM_CACHE[]::GAPGroupHomomorphism{FPGroup, MatGroup{ZZRingElem, ZZMatrix}}
end

For one, because SL2Z, S and T are needed so often that it is slightly more convenient (and more readable) to write _SL2Z_fp() instead of gens(domain(_matrix_hom())). And also for the following reason: _coset_action_hom(G) also has SL2Z as its domain, and we might want to ensure that they really use the same object as their domain?

Comment thread experimental/ModularGroup/src/ModularGroup.jl
Comment thread experimental/ModularGroup/src/types.jl Outdated
@@ -0,0 +1,7 @@
@attributes mutable struct ModularGroup

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So a ModularGroup is not an Oscar.Group ? I assume that is intentional as it does not implement the Group interface. But perhaps it should?

It really isn't so clear to me what instances of this type are meant to be. OTOH they are matrix groups (by the description above), but OTOH they do not seem to contain matrices... huh

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No, not implementing the Group interface was not intentional; just an embarrassing oversight. I changed this in 3edbfa5. I am, however, entirely unsure if I did it correctly. I'd appreciate some feedback, thanks!

Comment thread experimental/ModularGroup/src/types.jl Outdated
Comment thread experimental/ModularGroup/src/ModularGroup.jl Outdated
Comment thread experimental/ModularGroup/src/ModularGroup.jl Outdated
Comment thread experimental/ModularGroup/src/ModularGroup.jl Outdated
@Sebas777-gif
Sebas777-gif requested a review from fingolfin August 11, 2026 13:06
@Sebas777-gif

Copy link
Copy Markdown
Author

For completeness, I wanted to mention that parts of the code were generated by ChatGPT 5.6 Sol and Grok 4.5 and manually checked by me.

Sebas777-gif and others added 3 commits August 19, 2026 21:09
* Introduce shortcut for collecting exponent vectors for fine gradings (oscar-system#6063)

* Documentation on artifacts (oscar-system#5989)

Co-authored-by: JohnAAbbott <124266874+JohnAAbbott@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
Co-authored-by: Max Horn <max@quendi.de>

* Adds new `MultiFileRefSerializer` for serialization of references into separate files (oscar-system#6034)

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Martin Bies <HereAround@users.noreply.github.com>
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

* Add `on_graph` for acting on a graph with a `PermGroupElem` (oscar-system#6058)

* Some more improvements for `OFPModule`s (oscar-system#6061)

* Update GAP.jl to 0.17, i.e. GAP to 4.16.0 (oscar-system#6054)

Co-authored-by: Morgan Rodgers <morgan.joaquin@gmail.com>
Co-authored-by: Max Horn <max@quendi.de>

* Improve performance of `in(..., ::GSetByElements)` in some cases (oscar-system#6057)

* Add `is_feasible(::TropicalPolyhedra)` (oscar-system#6056)

Co-authored-by: Martin Bies <MBies87@googlemail.com>

* AI instructions (oscar-system#6042)

Co-authored-by: Martin Bies <MBies87@googlemail.com>

* Update LICENSE.md (oscar-system#6079)

* Remove another `>` in LICENSE (oscar-system#6081)

* Jaa/add nemo/matrix.md to doc (oscar-system#6078)

Co-authored-by: Martin Bies <HereAround@users.noreply.github.com>

* Bump codecov/codecov-action from 6 to 7 (oscar-system#6091)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump actions/checkout from 6 to 7 (oscar-system#6092)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* More details to the linear algebra introduction page (oscar-system#6083)

Co-authored-by: ChatGPT (OpenAI GPT-5.5) <noreply@chatgpt.com>
Co-authored-by: JohnAAbbott <124266874+JohnAAbbott@users.noreply.github.com>

* Align and link commutative algebra and linear algebra modules introduction page (oscar-system#6084)

Co-authored-by: JohnAAbbott <124266874+JohnAAbbott@users.noreply.github.com>

* Improvement of matroid realization space code (oscar-system#6029)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

* covector_decomposition: fix properties of output object (oscar-system#6097)

* Add `regular_action_homomorphism` function for a finite group `G` (oscar-system#6068)

* Replace `generators` by `_gens_of_null_cone` (oscar-system#6086)

* Fix link to Nemo page

* Bump Nemo compat

* Add `n_rays_modulo_lineality` (oscar-system#6087)

* Add `n_minimal_faces` (oscar-system#6089)

* Combinatorics: added copy(::Graph) (oscar-system#5930)

Co-authored-by: antonydellavecchia <antonydellavecchia@gmail.com>
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

* fix: recognize DEV-version files that need serialization upgrade (oscar-system#6099)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>

* Label Dependabot PRs as not needing release notes (oscar-system#6107)

* Add `dual` for roots and coroots (oscar-system#6098)

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>

* Add `visualize(::TropicalPolyhedron)` (oscar-system#6090)

* Add changelog from 1.7.3 to master (oscar-system#6112)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: changelog[bot] <changelog[bot]@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>

* Add conversion methods between OSCAR and HomotopyContinuation as a julia extension (oscar-system#5758)

* docs: fix some typos (oscar-system#6118)

Signed-off-by: John E <jeis4wpi@outlook.com>
Co-authored-by: Martin Bies <HereAround@users.noreply.github.com>

* Speed up realization space computation (oscar-system#6117)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

* chg: groebner_basis (tropical), optimization for homogeneous ideals (oscar-system#5978)

Co-authored-by: Oskar Henriksson <oskar.david.henriksson@gmail.com>

* move LeGreuelFormulaOnStratifiedSpaces.jl to extra long tests (oscar-system#6123)

* Update changelog for 1.8.0 (oscar-system#6113)

Co-authored-by: changelog[bot] <changelog[bot]@users.noreply.github.com>
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

* Set version to 1.9.0-DEV (oscar-system#6125)

* fix serialization version in test (oscar-system#6126)

* Add developer documentation of extra long tests

Co-authored-by: ChatGPT (OpenAI GPT-5.5) <noreply@chatgpt.com>

* refactor: computations of primitive embeddings and extensions of `ZZLat` (oscar-system#5949)

Co-authored-by: Stevell Muller <muller@cit-c0470eb29751.cit.uni-hannover.de>

* Collect group theory related types in `Groups/types.jl` (oscar-system#6106)

* Add information on OSCAR timing dashboard to developer documentation (oscar-system#6121)

Co-authored-by: Aaruni Kaushik <aaruni96@users.noreply.github.com>

* Add `cycle_graph(::Int)` (oscar-system#6137)

Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

* Add `graph_from_edges(::Polyhedron)` (oscar-system#6136)

* Warn when booktests change (oscar-system#6108)

Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Martin Bies <HereAround@users.noreply.github.com>
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

* Fix test statistics outside Git checkouts (oscar-system#6145)

Co-authored-by: OpenAI Codex <codex@openai.com>

* Update linear algebra structure to AA changes

* Fix `dual(::RootSpaceElem)` for a zero input (oscar-system#6147)

* document subtleties of `GAP.Globals` vs. `GAPWrap` (oscar-system#6153)

* Prevent mutations during F-theory model transformations (oscar-system#6149)

Co-authored-by: OpenAI Codex <codex@openai.com>

* Inform on updating policy for timing-tool

* Simplify FTheoryTools keyword documentation

Keep docstring signatures concise and remove keyword arguments from Documenter method references, while retaining the keyword explanations with each function. Complete the literature-model keyword list and clarify the exceptional-coordinate option for blowups.

Co-authored-by: Codex <codex@openai.com>

* Propagate flux-family computation options consistently (oscar-system#6162)

Co-authored-by: OpenAI Codex <codex@openai.com>

* Add coverage for concrete-base specialization

Exercise generic Weierstrass and parametrized Tate specialization over a concrete toric base, including coefficient grading, the restricted Tate vanishing condition, and input validation.

Co-authored-by: Codex <codex@openai.com>

* Name nested FTheoryTools data structures

Centralize aliases for resolution centers, resolution metadata, section input shapes, and string-keyed model data. Use the aliases consistently across model construction, tuning, specialization, getters, and mutators without changing stored representations.

Fixes oscar-system#4941

Co-authored-by: Codex <codex@openai.com>

* Update changelog for 1.8.1 (oscar-system#6178)

Co-authored-by: changelog[bot] <changelog[bot]@users.noreply.github.com>

* Group developer documentation pages (oscar-system#6148)

* Add `character_table_of_direct_product` (oscar-system#6169)

* Improve FTheoryTools performance and model-data safety (oscar-system#6151)

Co-authored-by: OpenAI Codex <codex@openai.com>

* Refresh zero-section class after toric blowups

Recompute the zero-section cohomology class in the post-blowup ambient ring instead of copying the pre-blowup value.

Co-authored-by: Codex <codex@openai.com>

* Update to mrdi file schema (oscar-system#6129)

* Test Stats: Account for an extra "_" in extra long tests (oscar-system#6181)

* Use Hecke orbit stabilizer for matrix groups over GF(2) (oscar-system#6138)

* Add `Combinatorics/intro.md` to the docs navigation (oscar-system#6184)

* Hardcode an isometry in a test (oscar-system#6185)

* docs: fix 13 broken refs (oscar-system#6183)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

* fix: primitive embeddings of even lattice into odd unimodular ones (oscar-system#6141)

Co-authored-by: Stevell Muller <muller@cit-c0470eb29751.cit.uni-hannover.de>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: John E <jeis4wpi@outlook.com>
Co-authored-by: Matthias Zach <85350711+HechtiDerLachs@users.noreply.github.com>
Co-authored-by: Martin Bies <HereAround@users.noreply.github.com>
Co-authored-by: JohnAAbbott <124266874+JohnAAbbott@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: antonydellavecchia <antonydellavecchia@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
Co-authored-by: Morgan Rodgers <morgan.joaquin@gmail.com>
Co-authored-by: Johannes Schmitt <johannes.schmitt@ruhr-uni-bochum.de>
Co-authored-by: Yue Ren <yue.ren.kl@gmail.com>
Co-authored-by: Martin Bies <MBies87@googlemail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: ChatGPT (OpenAI GPT-5.5) <noreply@chatgpt.com>
Co-authored-by: LukasKuehne <LukasKuehne@users.noreply.github.com>
Co-authored-by: long-zm123 <132650117+long-zm123@users.noreply.github.com>
Co-authored-by: Janika Peters <janika.peters@rwth-aachen.de>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: changelog[bot] <changelog[bot]@users.noreply.github.com>
Co-authored-by: John Eismeier <42679190+jeis4wpi@users.noreply.github.com>
Co-authored-by: Oskar Henriksson <oskar.david.henriksson@gmail.com>
Co-authored-by: Stevell Muller <78619134+StevellM@users.noreply.github.com>
Co-authored-by: Stevell Muller <muller@cit-c0470eb29751.cit.uni-hannover.de>
Co-authored-by: Aaruni Kaushik <aaruni96@users.noreply.github.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Thomas Breuer <sam@math.rwth-aachen.de>
Co-authored-by: Simon Brandhorst <brandhorst@math.uni-sb.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request experimental Only changes experimental parts of the code release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants