Skip to content

Clarify genus 2 minimal/simplified model snippets - #21

Closed
roed-math wants to merge 4 commits into
mainfrom
ai/t27-g2c-snippet-comments
Closed

Clarify genus 2 minimal/simplified model snippets#21
roed-math wants to merge 4 commits into
mainfrom
ai/t27-g2c-snippet-comments

Conversation

@roed-math

@roed-math roed-math commented Jul 19, 2026

Copy link
Copy Markdown
Owner

The genus 2 curve-page snippets defined a C (minimal model) and an X (simplified model)
without saying which was which. Following the suggestion in the issue, the snippets now use
Cmin and Csim with // minimal equation / // simplified equation comments, and the
simplified-model snippet displays the resulting curve. The simplified-model rational points
snippet now constructs points on Csim rather than on the minimal model, and the download file
gains a matching "Simplified equation" step. Along the way this fixes a bug in
simplify_hyperelliptic_point/comp_poly (y-coordinate divided by the content n of 4f+h²
instead of sqrt(n/squarefree_part(n))): for the 3160 curves with h = 0 the displayed
simplified-model points were not actually on the curve (e.g. (0 : -1/2 : 1) on 400.a.409600.1),
and the corrected points are needed for the new snippet to run. Addresses LMFDB#5344.

Also included, since it lands on the same cond line of code.yaml: the Magma download failed
for every curve with 4096 | abs_disc. Magma will not apply Ogg's formula once v_2(disc) >= 12, so
the conductor snippet has to pass it the local L-factor at 2 via ExcFactors. The curve page did
that, but the download built its conductor line from the static cond entry in code.yaml, which
had no such option, so downloads for those curves died with

Runtime error: v_2(disc)=14>=12, cannot use Ogg's formula unless forced by LocalData:="Ogg"

The option now comes from a single magma_cond_option() used by both the page snippet and the
download, with the code.yaml entry taking a placeholder the way curve already does, so the two
cannot drift apart again. This bug is pre-existing and independent of the renaming above.

Verified by running the generated Magma/Sage snippets for 169.a.169.1, 400.a.409600.1 and
336.a.172032.1, running the full regenerated Magma downloads for 400.a.409600.1 and 336.a.172032.1
end to end (both now complete and report the conductors and discriminants matching their labels),
rendering the affected pages and the Magma download with the test client, and the genus2_curves
test suite (46 passed).

Note that Magma prints WARNING: ExcFactors and BadPrimes are obsolete, use LocalData for the
affected curves. That warning is pre-existing on the curve page, and switching to the modern
LocalData spelling is left for a follow-up rather than changing page behavior here.

🤖 Generated with Claude Code

roed314 and others added 3 commits July 19, 2026 03:00
Rename C/X to Cmin/Csim in the genus 2 curve code snippets and add
// minimal equation and // simplified equation comments, so it is clear
which model each variable refers to; display Csim after defining it.
Point the simplified-model rational points snippet at Csim instead of
Cmin, and fix simplify_hyperelliptic_point/comp_poly to divide the
y-coordinate by sqrt(n/squarefree_part(n)) rather than n, so the
displayed simplified-model points (and the snippet) actually lie on the
simplified model when h = 0.  Mirror the renaming in the Code-to-Magma
download and add a Simplified equation step to it.

Verified by running the generated Magma snippets for 169.a.169.1,
400.a.409600.1 and 336.a.172032.1 (all pass except pre-existing Magma
package issues), the sage snippets in a sage REPL, page renders and the
Magma download via the flask test client, and the genus2_curves test
suite (42 passed).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mordell-Weil generators are stored with denominators cleared, as a relation
d*y = yD(x,z).  The y-coordinate of the minimal model transforms to the
simplified model as y -> (2*y + h(x,z))/s, so multiplying through by d gives
d*Y = (2*yD + d*h(x,z))/s: h picks up the same factor d.  comp_poly added
only a single copy of h, which was wrong for every curve having both h != 0
and a generator with a denominator (700 of the 4039 curves sampled).  On
336.a.172032.1 the generator 3x^2 - 32z^2 = 0, 6y = -35xz^2 was displayed on
the simplified model as 6y = x^3 - 69xz^2 rather than 6y = 6x^3 - 64xz^2,
which is 2x(3x^2 - 32z^2) as it must be for a 2-torsion generator.

comp_poly now takes the denominator as an argument, and mw_gens_table takes
a function of it, leaving the minimal-model path untransformed.

Adds regression tests for this and for the scale correction earlier in the
branch: 400.a.409600.1 (h = 0, content 4, so points scale by 2 and not 4)
and 336.a.172032.1, plus a check that the Magma download defines Cmin, Csim
and J in dependency order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@roed-math

Copy link
Copy Markdown
Owner Author

Thanks for the careful review. Fixed in f546785, which keeps the Cmin/Csim naming, the added simplified-model download step, and the corrected scale s exactly as they were.

The yden bug

Generators are stored with denominators cleared, as d*y = yD(x,z). Since y -> (2*y + h(x,z))/s, multiplying through by d gives

d*Y = (2*yD + d*h(x,z)) / s,

so h picks up the same factor d. comp_poly was adding a single copy of h, which is wrong exactly when h != 0 and d > 1.

comp_poly(fh, d) now takes the denominator, and mw_gens_table takes comp as a function of it (None on the minimal-model path, which is therefore untouched). Since yD is reassigned in place, the transformed expression is also what the D0 point-matching uses.

On 336.a.172032.1 the generator 3x^2 - 32z^2 = 0, 6y = -35xz^2 now displays on the simplified model as

6y = 6x^3 - 64xz^2

instead of the old 6y = x^3 - 69xz^2. As you noted, this is the 2-torsion consistency check: 6x^3 - 64xz^2 = 2x(3x^2 - 32z^2), so it does vanish modulo the x-part.

Tests

Three new tests in test_genus2_curves.py, all of which fail before this commit and pass after:

  • test_simplified_model_points: 400.a.409600.1, where h = 0 and content(4f+h^2) = 4. Asserts s == 2, that the four points survive the transform unchanged, and (algebraically, so it runs in CI without Magma) that each lands on Csim. Also checks the page builds them on Csim, that (0 : -1/2 : 1) is gone, and that no 1/2 creeps into the simplified Mordell-Weil table.
  • test_simplified_model_mw_gens: 336.a.172032.1. Direct algebraic assertion on the transform with d = 6 and h_hom = x^3 + xz^2, checking both the value and the 2x(3x^2 - 32z^2) factorization, plus page-level assertions that 6x^3 - 64xz^2 appears and x^3 - 69xz^2 does not.
  • test_model_code_snippets: the Magma download defines Cmin, then Csim := SimplifiedModel(Cmin), then J := Jacobian(Csim) in that order, and no bare C![...] survives on the page.

Validation

  • sage -python -m pytest lmfdb/genus2_curves/test_genus2_curves.py: 45 passed.
  • pyflakes and ruff clean on lmfdb/genus2_curves/.
  • Magma runs the page snippets for all three curves. The simplified points for 400.a.409600.1 come back as (0 : -1 : 1), (0 : 1 : 1), (1 : -1 : 0), (1 : 1 : 0), and Csim for 336.a.172032.1 is y^2 = -3x^6 + 62x^4 - 299x^2 - 224 as expected. The 169.a.169.1 download runs end to end.
  • Swept 4039 curves from g2c_ratpts: every transformed rational point satisfies the simplified equation, and every divisor found by D0 matching on the simplified model is the image of the one found on the minimal model. 700 of those curves have both h != 0 and a generator with a denominator, so they were displaying wrong relations before this fix.
  • GitHub Actions on f546785 is queued as I write this. The runs on the previously reviewed head (ee46714, the one your review flagged as still queued) did finish green, and the matrix takes about five hours on this repo, so I will follow up here when this one lands.

One thing left alone

The generated Magma download stops at Conductor(LSeries(Cmin)); for curves with v_2(disc) >= 12 (400.a.409600.1 and 336.a.172032.1 both hit this), with Magma refusing Ogg's formula. That is pre-existing and unrelated: the same failure happens on main with the old C := ... naming. The curve page snippet already works around it via ExcFactors, but the download's cond entry in code.yaml is a static string with no per-curve substitution, so it does not. Left for a separate PR rather than widening this one.

@roed314

roed314 commented Aug 5, 2026

Copy link
Copy Markdown

GPT signed off.

Magma refuses to apply Ogg's formula once v_2(disc) >= 12, so for those
curves the conductor snippet passes the local L-factor at 2 explicitly.
The snippet on the curve page did this, but the download built its
conductor line from the static entry in code.yaml, so downloads for
curves with 4096 | abs_disc died at Conductor(LSeries(Cmin)) with

    Runtime error: v_2(disc)=14>=12, cannot use Ogg's formula
    unless forced by LocalData:="Ogg"

Move the option into magma_cond_option() and have both callers use it,
with the code.yaml entry taking a placeholder the way 'curve' already
does.  Verified in Magma that the downloads for 400.a.409600.1 and
336.a.172032.1 now run to completion and report the right conductor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@roed314

roed314 commented Aug 5, 2026

Copy link
Copy Markdown

GPT signed off again.

@roed-math

Copy link
Copy Markdown
Owner Author

Superseded by LMFDB#7157, opened upstream from this same branch. Closing here; review continues upstream.

@roed-math roed-math closed this Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants