Reorganize extended examples and minor updates

[sujeetksb]

Changes include:

• Removed older example notebooks and added new paper example notebooks.
• Added an ambient_group function to simplify several computations.
• Updated the random element function to include the lift.
• Fixed one doctest in the extended module.

[fredstro]

Code Review: PR #16 — Reorganize extended examples and minor updates

Overview

Two-part PR: (1) replaces 9 ad-hoc example notebooks (Ext_Example*, other_exm*) with 5 cleanly-named ones (Example_coset, Example_cusp, Example_reduction{1,2,3}); (2) refactors extended/group_class.py and extended/pullback.py around a new ambient_group() concept (the group with level_ideal = O_K).

Net source changes: +170 / -211 lines (excluding notebooks). Notebooks: +5900 / -5900 — they dominate diff size but are pure noise to review.

What's Good

• ambient_group() abstraction (group_class.py:303, pullback.py:167) — cleanly captures that many pullback/cusp computations depend only on the ambient group, not on the level. Replaces ad-hoc ExtendedHilbertModularGroup(K, lattice_ideal, ...) reconstructions inside pullback.py and at the start of reduce() — a real conceptual cleanup.
• Dead-code removal in pullback.py — three large commented-out blocks (basis_matrix_ideal_on_power_basis, coordinates_in_number_field_ideal, basis_matrix_ideal__norm) deleted. Good.
• Generator construction split out into R(d) (group_class.py:445) — extracts the per-residue lift logic from generators(). Cleaner.
• Notebook renaming improves discoverability.
• .gitignore additions are sensible (caveat: .idea/ is editor-specific and usually belongs in personal global gitignore, not the repo).

Bugs / Correctness Issues

1. random_element uses u where d was intended (group_class.py near line 606)

return self(self.R(u) * self.E(u) * self.T(a) * self.L(b))

R expects an element coprime to level_ideal, but u is a unit. d (the freshly-sampled coprime residue) is computed but never used. Looks like R(d) was intended.

2. Breaking API rename without deprecation
The matrix_type literal "unit" was renamed to "Unit". The function docstring still says {'Lower', 'Upper', 'unit'} and doesn't mention the new "Lift". Either keep "unit" working or update the docstring.

3. coset_matrices identity logic changed silently
Old code generated all coset reps, then located an element that happens to lie in self and overwrote it with I. New code unconditionally inserts I when D == N. These are not obviously equivalent — please confirm and add a regression doctest, especially for non-trivial level_ideal where the old code's "find and replace" behavior mattered.

4. R(1) is now in the generators list
The new generators() loop iterates over all non-zero coprime residues including 1. R(1) produces a non-trivial lift like [1 0; 3 1] (visible in the updated doctest). That's not wrong — it's just an extra generator. Worth a comment confirming this is intentional rather than an off-by-one.

5. cusps() dangling expression converted to a comment, not removed

-                        -(1 - r) / c * g
+                        #a2 = -(1 - r) / c * g

This dead expression does nothing in either form. Either delete it or actually use a2. Leaving it as a commented-out assignment is the worst option — it implies relevance.

6. Determinant check in group_element.pyx:55

x.determinant() in parent.number_field().unit_group()

replaces x.determinant().is_unit(). This is more correct (is_unit() on a field element is always true for non-zero elements, so the prior check was effectively only is_totally_positive). But: (a) it's a real semantic fix worth calling out in the PR description, and (b) in K.unit_group() is materially slower than checking unit-of-O_K. Consider x.determinant() in parent.OK().unit_group() or an is_unit()-in-O_K equivalent.

Style / Minor

• d =1 — missing space.
• if coprime_residue == []: — prefer if not coprime_residue:.
• New R(d) docstring: "integer in number field coprime to d (default=1)" — should be "coprime to level_ideal" and there is no default.
• Indentation in the refactored coset_matrices else block uses 3-space indents — should be 4.
• Stray blank line after self._ambient_group = G.ambient_group() in pullback.py.__init__.
• New R decorator is @cached_method on a method keyed by a number-field element — make sure hashability/equality behave for the keys you actually pass (normally fine in Sage, just flag).

Tests

• New doctests for ambient_group() and R() — good.
• The generator/doctest diff reflects the new generator order. Fine, but doesn't verify the underlying generation is still a generating set (a programmatic check that the new gen list generates the same subgroup would catch regressions from the coset_matrices and generators refactors).
• No new tests for the substantial pullback.py semantic shift (group() → ambient_group() ~25 sites). Worth at least one regression doctest where self.group() != self.ambient_group() (i.e., non-trivial level_ideal) showing pullback results don't depend on the level — that's the whole point of the refactor.

Recommended Action

• Block on: items 1 (R(u) vs R(d)), 2 (API rename + docstring), and 3 (coset_matrices semantic change confirmation).
• Request before merge: items 5, 6 explanation, plus the style nits.
• Once those are addressed, the underlying ambient_group refactor is a nice improvement and worth landing.

[fredstro]

Pushed 4261e57 directly to this branch addressing the safe items from the review. Summary:

Fixed

• random_element: self.R(u) → self.R(d) (was passing a unit where a residue coprime to level_ideal is required).
• random_element: tidied d =1 → d = 1 and if coprime_residue == [] → if not coprime_residue.
• random_element: updated docstring to list current modes {'Lower','Upper','Unit','Lift'} and added structural doctests for each — including a "Lift" test with non-trivial level_ideal to exercise the bug above.
• R(d): docstring corrected to "coprime to level_ideal" (was self-referential; no default).
• cusps(): dropped dead #a2 = -(1 - r) / c * g line.
• coset_matrices(): fixed 3-space indentation in the refactored else block to 4 spaces.
• ExtendedHilbertPullback.__init__: removed stray double blank line.

Not changed — needs your input

1. "unit" → "Unit" API rename. Breaking change. Want to keep "unit" accepted (case-insensitive or alias) for back-compat, or commit to the new spelling?
2. coset_matrices identity insertion. New code inserts I unconditionally at D == N; old code generated the full list then found-and-replaced an element of self. Please confirm these are mathematically equivalent across all (lattice_ideal, level_ideal) combinations — a regression doctest at a non-trivial level would be reassuring.
3. group_element.pyx determinant check. x.determinant() in K.unit_group() is the right semantic fix but is significantly slower than the previous is_unit() (which was actually too permissive on field elements). Consider x.determinant() in parent.OK().unit_group() instead.

Also left untouched: the broader test-coverage gap for pullback.py's group() → ambient_group() shift — worth at least one regression doctest where self.group() != self.ambient_group() (non-trivial level) shows pullback results don't depend on the level.

Couldn't run the doctests locally — sage env hit a passagemath 10.5/10.8 ABI mismatch (cysignals._do_raise_exception). CI should cover it.

[fredstro]

@sujeetksb following up on point 2 from my fix-summary comment — could you confirm the coset_matrices semantic change is intended?

Old behavior (src/hilbert_modgroup/extended/group_class.py, deleted):

for D in divisors(N):
    # ... compute reps for every divisor D, including D == N ...
    for r in (N / D).residues():
        if I.is_coprime(r):
            # ... build a, b, c, d ...
            L.append(H.create_element(a, b, c, d))
for x in L:
    if x in self:
        idx = L.index(x)
        L[idx] = H.create_element(1, 0, 0, 1)
        break

The loop ran for every D ∈ divisors(N), then post-loop scanned the full list looking for the first element of L that lies in self and replaced it with the identity.

New behavior (added):

for D in divisors(N):
    if D == N:
        L.append(H.create_element(1, 0, 0, 1))
    else:
        # ... the original construction for D != N ...

The D == N branch is short-circuited to always append the identity. The post-loop find-and-replace is gone.

Why this might not be equivalent:

• The old code's find-and-replace assumed some generated element happens to be in self. The new code asserts the identity-producing element must come from the D == N case.
• When D == N, N/D = (1) and (N/D).residues() == [0], and the old construction with d = 0 hit the d.is_zero() branch producing [[1, -1/c], [c, 0]] — not the identity.
• So at D == N the new list contains I where the old list contained [[1, -1/c], [c, 0]]. Total list length psi(N) is preserved, but one specific representative differs.
• [[1, -1/c], [c, 0]] and I are not in the same coset in general, so the new list may no longer be a valid set of distinct coset representatives.

Two questions:

1. Is the new behavior intended, and if so, can you sketch why D == N always produces the identity coset?
2. Could you add a regression doctest at a non-trivial (lattice_ideal, level_ideal) where the old find-and-replace would have triggered? Even just asserting H.create_element(1, 0, 0, 1) in H.coset_matrices() and that the length is psi(N) would be enough.

If point 1 has a clean justification, fine to keep — but the change shouldn't go in silently.

[fredstro]

Follow-up on point 3 (the group_element.pyx determinant check) — a cheaper alternative than det in K.unit_group():

d = x.determinant()
inv = d ** -1
if not (d.is_integral() and inv.is_integral() and d.is_totally_positive()):
    raise TypeError("matrix must have determinant equal to totally positive unit")

d ∈ O_K^* iff d ∈ O_K and d^{-1} ∈ O_K, i.e. both are integral. is_integral() on a number field element just inspects its minimal polynomial coefficients — O(1) work — whereas in K.unit_group() materializes the unit group and does a discrete-log-style membership test against the unit generators. This check runs on every matrix construction so the cost adds up.

Equivalent in semantics to your current check, much cheaper per call.

[fredstro]

Ran the full extended subpackage doctest suite locally on commit 42c5fcd:

sage -t --force-lib src/hilbert_modgroup/extended/
  group_class.py        [241 tests, 1.50s]
  pullback.py           [748 tests, 2.10s]
  pullback_cython.pyx   [ 34 tests, 0.57s]
  group_element.pyx     [203 tests, 1.15s]
  __init__.py, all.py   [  0 tests]
  + cusp.py and friends pass
All tests passed!

1226 tests green including the new random_element matrix-type doctests and the ambient_group level-independence regression test.

[sujeetksb]

Thanks for pointing this out.

Yes, this change was intentional. My motivation was to avoid constructing a representative that would later be replaced by the identity anyway.

Looking at the D == N case more carefully, the code computes d = 1, and consequently a = 1 and b = 0, so the representative is [[1, 0], [c, 1]]. Moreover, (c) = Dp * lattice_ideal * N, and since Dp is an integral ideal, we have Dp * lattice_ideal * N ⊆ lattice_ideal * N, so c ∈ lattice_ideal * N. Hence this representative already lies in the level group and represents the identity coset.

So my intention was simply to insert the identity directly in this special case instead of constructing this representative first and replacing it afterwards.