Skip to content

Commit bc52f5a

Browse files
roed314claude
andcommitted
Load the ECNF base field polynomials in a single query
Review follow-up: ECNFDownloader looked up the defining polynomial of each curve's base field one label at a time (memoized with lru_cache). There are only 894 base fields occurring in ec_nfcurves, so fetch all of them from nf_fields in one search the first time a download happens and keep them in a dictionary on the downloader. postprocess now just indexes into that dictionary, and generating a download issues no further queries. Also extend the unknown-rank regression test to all six download formats, asserting on the rank column rather than on a whole serialized row, add a test that the polynomials are loaded in bulk and reused, and unit test the Oscar rational serialization in lmfdb/tests/test_utils.py. Downloading the 712 curves of conductor norm 1 in sage format against devmirror takes 2.25s including building the dictionary and 0.98s after. lmfdb/ecnf/test_ecnf.py (14 passed) and lmfdb/tests/test_utils.py (20 passed) pass; pyflakes and ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 4958a12 commit bc52f5a

3 files changed

Lines changed: 57 additions & 15 deletions

File tree

lmfdb/ecnf/main.py

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,15 @@
33

44
import ast
55
import re
6-
from functools import lru_cache
76
from urllib.parse import quote, unquote
87

98
from flask import render_template, request, url_for, redirect, make_response, abort
10-
from sage.all import factor, is_prime, QQ, ZZ, PolynomialRing
9+
from sage.all import factor, is_prime, lazy_attribute, QQ, ZZ, PolynomialRing
1110

1211
from lmfdb import db
1312
from psycodict.encoding import Json
1413
from lmfdb.utils import (
15-
to_dict, flash_error, display_knowl, Downloader,
14+
to_dict, flash_error, display_knowl, Downloader, coeff_to_poly,
1615
parse_ints, parse_ints_to_list_flash, parse_noop, nf_string_to_label, parse_element_of,
1716
parse_nf_string, parse_nf_jinv, parse_bracketed_posints, parse_floats, parse_primes,
1817
SearchArray, TextBox, SelectBox, CountBox, SubsetBox, TextBoxWithSelect,
@@ -449,17 +448,22 @@ class ECNFDownloader(Downloader):
449448
title = "Elliptic curves over number fields"
450449
short_name = "curves"
451450

452-
@staticmethod
453-
@lru_cache(maxsize=128)
454-
def field_poly(field_label):
455-
# Look up the defining polynomial of a base field; cached since download
456-
# results are sorted by field, so the same field shows up in consecutive rows
457-
from lmfdb.utils import coeff_to_poly
458-
return coeff_to_poly(db.nf_fields.lookup(field_label, projection='coeffs'))
451+
@lazy_attribute
452+
def field_polys(self):
453+
# The defining polynomials of the base fields, keyed by field label. There
454+
# are under a thousand base fields, so we fetch them all in one query the
455+
# first time a download happens rather than looking them up row by row.
456+
labels = db.ec_nfcurves.distinct('field_label')
457+
polys = {rec['label']: coeff_to_poly(rec['coeffs'])
458+
for rec in db.nf_fields.search({'label': {'$in': labels}}, projection=['label', 'coeffs'])}
459+
missing = [label for label in labels if label not in polys]
460+
if missing:
461+
raise ValueError("Missing defining polynomial for base field %s" % missing[0])
462+
return polys
459463

460464
def postprocess(self, row, info, query):
461465
# Look up the defining polynomial coefficients for the base field of the curve
462-
row["field_coeffs"] = self.field_poly(row['field_label'])
466+
row["field_coeffs"] = self.field_polys[row['field_label']]
463467

464468
# Convert Weierstrass coefficients from string to a list of list of rationals
465469
row['ainvs'] = [[QQ(aj) for aj in ai.split(",")] for ai in row['ainvs'].split(";")]

lmfdb/ecnf/test_ecnf.py

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,38 @@ def test_search_download(self):
9696
# For curves whose rank is not known, the rank bounds should be
9797
# downloaded rather than a LaTeX string such as "0 \le r \le 1"
9898
base = '/EllipticCurve/?download=1&query=%7B%27field_label%27%3A+%272.0.868.1%27%2C+%27conductor_norm%27%3A+2%7D&Submit='
99-
for lang in ['sage', 'oscar']:
99+
for lang in ['sage', 'gp', 'magma', 'text', 'csv', 'oscar']:
100100
L = self.tc.get(base + lang)
101101
data = L.get_data(as_text=True)
102-
assert '["2.1-b1", "2.1-b", "2.0.868.1", [217, 0, 1], 2, [0, 1], []' in data
103-
assert '"0 \\\\le r \\\\le 1"' not in data
102+
# The columns preceding the Sato-Tate group are the two labels, the base
103+
# field with its defining polynomial, the conductor norm, the rank and the
104+
# torsion, so [0, 1] can only be the bounds on the rank of this curve
105+
row = [line for line in data.split('\n') if '2.1-b1' in line][0]
106+
assert '[0, 1]' in row.split('1.2.A.1.1a')[0]
107+
assert r'\le r' not in row
108+
109+
def test_download_field_polys(self):
110+
r"""
111+
Check that the base field defining polynomials used when downloading
112+
search results are loaded in a single query (issue #7004)
113+
"""
114+
from unittest.mock import patch
115+
from lmfdb.ecnf.main import ECNFDownloader
116+
downloader = ECNFDownloader()
117+
rows = [{'field_label': label, 'ainvs': '0,0,0;0,0,0;0,0,0;0,0,0;0,0,0'}
118+
for label in ['3.3.1849.1', '2.0.868.1', '3.3.1849.1']]
119+
with patch.object(self.db.nf_fields, 'search', wraps=self.db.nf_fields.search) as search, \
120+
patch.object(self.db.nf_fields, 'lookup', wraps=self.db.nf_fields.lookup) as lookup:
121+
polys = [downloader.postprocess(row, {}, {})['field_coeffs'] for row in rows]
122+
# All of the defining polynomials are fetched by the first row, in bulk
123+
assert search.call_count == 1
124+
assert lookup.call_count == 0
125+
# and the dictionary they were saved in is reused afterwards
126+
assert downloader.field_polys is downloader.field_polys
127+
assert search.call_count == 1
128+
assert polys[0] is polys[2]
129+
for row, poly in zip(rows, polys):
130+
assert poly.degree() == int(row['field_label'].split('.')[0])
104131

105132
def test_search(self):
106133
r"""

lmfdb/tests/test_utils.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import unittest
1010

11-
from sage.all import var
11+
from sage.all import var, QQ
1212

1313
from lmfdb.utils import (
1414
an_list,
@@ -38,6 +38,8 @@
3838
infinity,
3939
)
4040

41+
from lmfdb.utils.downloader import OscarLanguage, SageLanguage
42+
4143
class UtilsTest(unittest.TestCase):
4244
"""
4345
An example of unit tests that are not based on the website itself.
@@ -104,6 +106,15 @@ def test_splitcoeff(self):
104106
self.assertEqual(splitcoeff(" 0 -1.2 \n 3.14 1 "),
105107
[[0.0, -1.2], [3.14, 1.0]])
106108

109+
def test_rational_to_lang(self):
110+
r"""
111+
Checking utility: DownloadLanguage.rational_to_lang
112+
"""
113+
# In Julia, -3/2 is floating point division, so Oscar needs -3//2
114+
self.assertEqual(SageLanguage().to_lang(QQ(-3) / 2), "-3/2")
115+
self.assertEqual(OscarLanguage().to_lang(QQ(-3) / 2), "-3//2")
116+
self.assertEqual(OscarLanguage().to_lang(QQ(3)), "3")
117+
107118
################################################################################
108119
# display and formatting utilities
109120
################################################################################

0 commit comments

Comments
 (0)