Skip to content

Commit

Permalink
gh-38361: EllipticCurve: Raise error on unexpected keyword argument
Browse files Browse the repository at this point in the history
    
As described in the title. Currently the constructor just silently
ignores the arguments it doesn't recognize, which can be confusing if
the user pass in `ring=R` and see it has no effect.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion. (not aware of one)
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.


### ⌛ Dependencies

None.
    
URL: #38361
Reported by: user202729
Reviewer(s): Kwankyu Lee, Lorenz Panny, user202729
  • Loading branch information
Release Manager committed Aug 27, 2024
2 parents e88b194 + eae2120 commit 4c8fdb7
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 11 deletions.
35 changes: 33 additions & 2 deletions src/sage/schemes/elliptic_curves/constructor.py
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,9 @@ def create_key_and_extra_args(self, x=None, y=None, j=None, minimal_twist=True,
# Interpret x as a Cremona or LMFDB label.
from sage.databases.cremona import CremonaDatabase
x, data = CremonaDatabase().coefficients_and_data(x)
# data is only valid for elliptic curves over QQ.
if R not in (None, QQ):
data = {}
# User-provided keywords may override database entries.
data.update(kwds)
kwds = data
Expand All @@ -457,7 +460,7 @@ def create_key_and_extra_args(self, x=None, y=None, j=None, minimal_twist=True,

return (R, tuple(R(a) for a in x)), kwds

def create_object(self, version, key, **kwds):
def create_object(self, version, key, *, names=None, **kwds):
r"""
Create an object from a ``UniqueFactory`` key.
Expand All @@ -467,18 +470,46 @@ def create_object(self, version, key, **kwds):
sage: type(E)
<class 'sage.schemes.elliptic_curves.ell_finite_field.EllipticCurve_finite_field_with_category'>
``names`` is ignored at the moment, however it is used to support a convenient way to get a generator::
sage: E.<P> = EllipticCurve(QQ, [1, 3])
sage: P
(-1 : 1 : 1)
sage: E.<P> = EllipticCurve(GF(5), [1, 3])
sage: P
(4 : 1 : 1)
.. NOTE::
Keyword arguments are currently only passed to the
constructor for elliptic curves over `\QQ`; elliptic
curves over other fields do not support them.
TESTS::
sage: E = EllipticCurve.create_object(0, (QQ, (1, 2, 0, 1, 2)), rank=2)
sage: E = EllipticCurve.create_object(0, (GF(3), (1, 2, 0, 1, 2)), rank=2)
Traceback (most recent call last):
...
TypeError: unexpected keyword arguments: {'rank': 2}
Coverage tests::
sage: E = EllipticCurve(QQ, [2, 5], modular_degree=944, regulator=1)
sage: E.modular_degree()
944
sage: E.regulator()
1.00000000000000
"""
R, x = key

if R is QQ:
from .ell_rational_field import EllipticCurve_rational_field
return EllipticCurve_rational_field(x, **kwds)
elif isinstance(R, NumberField):
elif kwds:
raise TypeError(f"unexpected keyword arguments: {kwds}")

if isinstance(R, NumberField):
from .ell_number_field import EllipticCurve_number_field
return EllipticCurve_number_field(R, x)
elif isinstance(R, sage.rings.abc.pAdicField):
Expand Down
25 changes: 17 additions & 8 deletions src/sage/schemes/elliptic_curves/ell_rational_field.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ def __init__(self, ainvs, **kwds):
TESTS:
Passing unexpected keyword arguments will raise an error::
sage: EllipticCurve.create_object(0, (QQ, (1, 2, 0, 1, 2)), base=QQ)
Traceback (most recent call last):
...
TypeError: unexpected keyword arguments: {'base': Rational Field}
When constructing a curve from the large database using a
label, we must be careful that the copied generators have the
right curve (see :issue:`10999`: the following used not to work when
Expand All @@ -181,21 +188,23 @@ def __init__(self, ainvs, **kwds):
EllipticCurve_number_field.__init__(self, Q, ainvs)

if 'conductor' in kwds:
self._set_conductor(kwds['conductor'])
self._set_conductor(kwds.pop('conductor'))
if 'cremona_label' in kwds:
self._set_cremona_label(kwds['cremona_label'])
self._set_cremona_label(kwds.pop('cremona_label'))
if 'gens' in kwds:
self._set_gens(kwds['gens'])
self._set_gens(kwds.pop('gens'))
if 'lmfdb_label' in kwds:
self._lmfdb_label = kwds['lmfdb_label']
self._lmfdb_label = kwds.pop('lmfdb_label')
if 'modular_degree' in kwds:
self._set_modular_degree(kwds['modular_degree'])
self._set_modular_degree(kwds.pop('modular_degree'))
if 'rank' in kwds:
self._set_rank(kwds['rank'])
self._set_rank(kwds.pop('rank'))
if 'regulator' in kwds:
self.__regulator = (kwds['regulator'], True)
self.__regulator = (kwds.pop('regulator'), True)
if 'torsion_order' in kwds:
self._set_torsion_order(kwds['torsion_order'])
self._set_torsion_order(kwds.pop('torsion_order'))
if kwds:
raise TypeError(f"unexpected keyword arguments: {kwds}")

def _set_rank(self, r):
r"""
Expand Down
2 changes: 1 addition & 1 deletion src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -2409,7 +2409,7 @@ def __init__(self, Q, R=None, invert_y=True):
if not hasattr(self, '_curve'):
if self._Q.degree() == 3:
ainvs = [0, self._Q[2], 0, self._Q[1], self._Q[0]]
self._curve = EllipticCurve(ainvs, check_squarefree=R.is_field())
self._curve = EllipticCurve(ainvs)
else:
self._curve = HyperellipticCurve(self._Q, check_squarefree=R.is_field())

Expand Down

0 comments on commit 4c8fdb7

Please sign in to comment.