Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hyperelliptic curves using the smooth model #39161

Draft
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Dec 18, 2024

Overview

This PR includes a whole new module into schemes which implements hyperelliptic curves over the smooth model, and focuses on implementing decent arithmetic for Jac(H) for all reasonable cases, something which is not done properly in the current implementation due to limitations of the curve model used.

The idea is that hyperelliptic_curves_sm will be available with the old model for some time, until someone decides we should depreciate the old model in favour of this new implementation.

Motivation

The current implementation of hyperelliptic curves in SageMath uses the projective plane model. Although this works nicely enough for imaginary curves with only one point at infinity, it is not descriptive enough for the real models. My gut feeling is the projective plane model was used as in early Sage days this was easier to do and allowed hyperelliptic curves to be supported at all. Mathematically, I believe it makes much more sense to use a weighted projective model to have this new description but it requires more tools which we only have thanks to the work of others in sage since it was released.

This PR is a total rewrite of the hyperelliptic curve classes to instead use the smooth model for hyperelliptic curves which facilitates implementing arithmetic of Jacobians of hyperelliptic curves for (almost) all cases. In particular, now if one tries to perform arithmetic on Jac(C) one either gets the correct value or a well-handled error instead of just returning something which is wrong.

The hope is that with this new model for the curves, more recent research in the area of hyperelliptic curves and their jacobians can be more easily integrated. As a personal example, being able to compute arithmetic in Jac(H) for the real model unconditionally would help with the implementation of genus two isogenies which are "in vogue" right now in the cryptography world.

Content of PR

This PR looks WAY bigger than it is, as it duplicates ALL code from hyperelliptic_curves and then has modifications to allow this to work in the new curve model. I believe the best thing to do in the long run is to depreciate the old hyperelliptic curve impl and have this replace it, but this will take years(?) so I think we'll just have to have both side by side for a while.

This PR still needs a lot of work including:

  • more tests
  • better comments and docstrings
  • potentially refactoring of which files belong where

but the code has sat at this stage for a long time without much more progress from the three of us, so I think the best thing to do is open up the PR and try and get some extra attention on getting this code ready to be included.

One benefit is that all this code is "new" in that it should not conflict with anyone else's work, so we can get this code into a good position and keep adding to it with more interesting maths once the base layer is in place

Example of better arithmetic

For example, this code fixed the following issue:

#32024

sage: R.<x> = QQ[]
sage: f = 144*x^6 - 240*x^5 + 148*x^4 + 16*x^3 - 16*x^2 - 4*x + 1
sage: H = HyperellipticCurveSmoothModel(f)
sage: J = Jacobian(H)
sage: P = J(H(0,1))-J(H(0,-1))
sage: (5*P).is_zero()
False
sage: 
sage: H = HyperellipticCurve(f)
sage: J = Jacobian(H)
sage: P = J(H(0,1))-J(H(0,-1))
sage: (5*P).is_zero()
True

and is also related to the issues #37109 #37093 #37101 #37626

Help!

This code needs more work, and lots of time spent on the review which I don't think is a reasonable thing to do for one person. So if this area interests you any help would be appreciated (either in the review of some of the code or some extra commits which tidy up aspects)

Copy link

github-actions bot commented Dec 18, 2024

Documentation preview for this PR (built with commit 34dd9ac; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729
Copy link
Contributor

Not an expert here, but why exactly would it be backwards incompatible to just replace the old class with the new one entirely? (can't you find some reasonable bijective correspondence between old and new model?)

@GiacomoPope
Copy link
Contributor Author

there's some details about it here: https://groups.google.com/g/sage-devel/c/kIwcYIzm4is/m/b3eefNSzAQAJ

generally, my understanding is that if a user is running code with hyperelliptic right now and we dropped this in to replace it, there's a good chance their code would either not work, or work in an unexpected way and so we must do the proper depreciation.

In my mind that means

  1. Add this code in without any warnings
  2. Add a depreciation warning to hyperelliptic once this is feature full
  3. Once the depreciation time is elapsed, make hyperelliptic the hyperelliptic_sm. Also keep hyperelliptic_sm and add a new depreciation to hyperelliptic_sm warning to move to hyperelliptic
  4. Delete hyperelliptic_sm and have only hyperelliptic which is this new code

@GiacomoPope
Copy link
Contributor Author

@grhkm21 looks like your most recent commits broke other doctests

We chose not to normalise all points, but rather just the points where
the weight of the coordinate corresponding to the last nonzero entry of
the points
@user202729
Copy link
Contributor

Do you think it's worth making a global flag hyperelliptic_curve.use_smooth_model(True/False) (so e.g. it takes less time to deprecate things, for example if an user now want to use the smooth model they need to type SmoothModel everywhere, then after deprecation period they need to delete that everywhere)

Of course in that case whatever the global variable is, the user can always import explicitly from one of the two module to select which to use regardless.

Some cursory notes:

  • in

     +    The construction of hyperelliptic curves is supported over different fields. The correct class is chosen automatically::
     +        sage: F = FiniteField(13)
     +        sage: S.<x> = PolynomialRing(F)
     +        sage: HF = HyperellipticCurve(x^5 + x^4 + x^3 + x^2 + x + 1, x^3 + x); HF
     +        Hyperelliptic Curve over Finite Field of size 13 defined by y^2 + (x^3 + x)*y = x^5 + x^4 + x^3 + x^2 + x + 1
     +        sage: type(HF)
     +        <class 'sage.schemes.hyperelliptic_curves.constructor.HyperellipticCurve_g2_FiniteField_with_category'>

    This still uses the old class? (a few more examples below)

  • I don't think it's a good idea to copy the docstring between the module and the def HyperellipticCurveSmoothModel. Since the user is most likely to type HyperellipticCurveSmoothModel? in the sage command-line it seems best to put all these in the function docstring, then in the module docstring type See :func:`HyperellipticCurveSmoothModel` for more details.

  • in

     +        # Ensure we are adding two divisors
     +        assert isinstance(other, type(self))

    no harm, but redundant. The coercion model ensures _add_ is only ever called on elements with same parent.

  • in

     +    def _richcmp_(self, other, op):
     +        """
     +        Test the weighted projective equality of two points.
     +        ...
     +        return richcmp(self._coords, other._coords, op)

    this feels dangerous (no total ordering induced on points?) since there is normalize_coordinates() implemented I guess that can be used (is the performance hit worth it?)

  • in

     +    def __hash__(self):
     +        """
     +        Computes the hash value of this point.
     +
     +        OUTPUT: Integer.
     +
     +        EXAMPLES::
     +
     +            sage: P.<x,y> = ProjectiveSpace(QQ, 1)
     +            sage: hash(P([1/2, 1])) == hash(P.point([1, 2], False))
     +            True
     +        """
     +        P = copy(self)
     +        P.normalize_coordinates()
     +        return hash(tuple(P))
     +
    

    There's something that feel morally wrong. Either normalize_coordinates is completely harmless and it's free to invoke it everywhere (including _repr_), then the copy is not necessary, or it dangerously mutates the object (in which case it seems better to make as many things immutable as possible). I prefer the former.

@GiacomoPope
Copy link
Contributor Author

A lot of these points are very good, thank you. I'll try and fix them all after the holidays!

@grhkm21
Copy link
Contributor

grhkm21 commented Dec 25, 2024

@GiacomoPope can you mark this PR as draft?

@GiacomoPope GiacomoPope marked this pull request as draft January 6, 2025 11:22
@GiacomoPope
Copy link
Contributor Author

@grhkm21 I have marked it as a draft PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants