Do not inherit from hyperelliptic curves for padic elliptic curve code

[GiacomoPope]

In the much larger PR #39161 there is an effort to move from projective space to weighted projective space $$(1 : g + 1 : 1)$$ for hyperelliptic curves. Among many other things, making this change allows for correct arithmetic in Jacobian(H) which currently fails for many curves.

When making this change, it was noticed that to access the coleman integral, EllipticCurve_padic_field inherits HyperellipticCurve_padic_field:

class EllipticCurve_padic_field(EllipticCurve_field, HyperellipticCurve_padic_field):
    ....

to access many useful functions. However, it doesnt make sense for elliptic curves to be over weighted projective space and so there's a tension between this inheritance and the work in #39161.

The proposal here is to move the code from the current implementation of HyperellipticCurve_padic_field into EllipticCurve_padic_field and then when HyperellipticCurve_padic_field changes model, the EllipticCurve_padic_field methods will be unchanged.

There are many TODO here as in moving from the hyperelliptic curve to elliptic curve code, we lose many docstrings and examples which need to be repopulated. This code is also old and could do with some general clean up.

Before I start cleaning this up and doing more work, I want to know whether this kind of PR would be accepted in helping #39161 move forward.

[JohnCremona]

It would be good to have the help of someone who was involved in writing the p-adic elliptic curve code, or someone who uses it. Not me!

[GiacomoPope]

I've asked Sabrina for help on this PR who is experienced with writing and using this code (she's the one who helped with the PR for the new hyperelliptic curve code in the weighted projective model).

There's two things to do here:

1. Clean up the code generally, as it's about 15 years old
2. Update the functions to explictly be genus 1 and as fast as possible

I can handle 1, I think from the sage/python point of view and I hope Sabrina can help with 2.

I suppose the question from the "community" which I have is whether it's OK for there to be this code duplication in ell and hyperell due to the impending different projective spaces.

[github-actions]

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

[GiacomoPope]

Now I can see the CI is passing with the first sketch of this PR I will start refactoring the code to try and follow more model sagemath guidelines.

[vincentmacri]

I suppose the question from the "community" which I have is whether it's OK for there to be this code duplication in ell and hyperell due to the impending different projective spaces.

I do not object to code duplication as a temporary measure until #39161 is merged. I think it's a good approach. You could also make use of helper functions if it's duplication of something that isn't getting changed in #39161.

I can help review this, although I haven't worked with curves over p-adic fields before. It's close enough to my research area that I don't mind an excuse to learn about it. If you have any recommended references on the subject for someone who is familiar with elliptic curves but not over p-adic fields that would be helpful for me to review this.

From your comments, I take it you're looking for feedback but not a full review yet? You can mark the PR as a draft until it's ready for a full review. I'll still answer any discussion questions here while it's a draft, but know not to take a detailed look at the code until it's ready that way.

[vincentmacri]

And just to be sure, the only sense in which this PR would change behaviour is that if H is a EllipticCurve_padic_field then isinstance(H, HyperellipticCurve_padic_field) will change from True to False after this PR?

[GiacomoPope]

I could mark it as a draft now, it was not in this state before as I wanted the CI to run everything when I originally removed functionality from the old class.

I don't have a good resource and the p-adic stuff is something I do not know much about.

One of the core dependencies is this monsky_washnitzer.py file, but Sabrina explained to me that this is really in the Affine model, and if you look at #39161 then you can see the only changes of this file are cosmetic so I expect this to go smoothly.

For this PR to be merged now is a matter of aesthetic I believe. The code works for genus one and is of the same form as the code in the hyperelliptic branch. A criticism could be made of the code itself which maybe is ready for a proper review for.

The mathematical work is in

1. Putting examples in the docstrings, I think maybe getting examples from Magma is a good idea, but we could also just generate them from HyperellipticCurve with f(x) as a genus one curve
2. Working on optimising the code now we're in g = 1 with dim = 2, there are several places.

My daughter is sick at the moment so I do not think I will work on this over the weekend so please do as little or as much as you like. Sabrina said she would also look at it but from my cursory look I'm not sure there's much to optimise in here after all and really this is about cleaning up the old code to modern SageMath standards

[GiacomoPope]

And just to be sure, the only sense in which this PR would change behaviour is that if H is a EllipticCurve_padic_field then isinstance(H, HyperellipticCurve_padic_field) will change from True to False after this PR?

Not only this, but also because of this inheritance EllipticCurve_padic_field had access to all the methods in HyperellipticCurve_padic_field and HyperellipticCurve_genetic which I think was never really well documented or intended. If someone worked with EllipticCurve_padic_field explicitly and relied on these hyperelliptic curve functions (ones which exists for H but are not overwritten by the elliptic curve methods) then something could break

Some common ones (like hyperelliptic_polynomials) are already in EllipticCurve and I have introduced all other necessary methods to ensure Doctests pass, but some non-documented functions could now fail I suppose...

[vincentmacri]

Some common ones (like hyperelliptic_polynomials) are already in EllipticCurve and I have introduced all other necessary methods to ensure Doctests pass, but some non-documented functions could now fail I suppose...

If all existing doctests pass without changes then I think that's good enough that it's unlikely this is breaking anything.

By non-documented functions do you mean private methods (i.e. _foo)? Those are totally fine to break. Or do you mean public methods that came from inheritance? For the latter, what methods would we have for hyperelliptic curves but not elliptic curves that someone would reasonably want for elliptic curves? Aren't elliptic curves essentially just hyperelliptic curves of genus 1?

[vincentmacri]

I could mark it as a draft now, it was not in this state before as I wanted the CI to run everything when I originally removed functionality from the old class.

CI will run on a draft PR.

In terms of how much you need to do to clean up old code to our current standards, when it comes to refactoring old code, my standards are lower than for new code. Refactoring is important but not very interesting work, so as long as the code quality is non-decreasing and you aren't doing something that will make cleaning up the code harder later on I am okay with it. Of course it is nice if you can clean up the old code while you're doing the refactoring, but I will not require you to do that for a positive review.

In terms of optimizations, as long as you aren't doing anything that degrades performance it's fine. If you've identified areas that can be optimized, you can add a TODO note in the code and optionally file an issue and tag the issue with t: performance.

If you just want to copy all the needed functions from hyperelliptic curves to elliptic curves (and adapt the doctests accordingly) and remove the inheritance, and leave the rest of the cleanup and optimizations for future work, that's fine by me. A TODO comment and opening an issue (or issues) for what that future work you've identified is would be appreciated in that case.

[GiacomoPope]

It would mean cross referencing the API for hyperelliptic generic and the generic elliptic curve. I can do this if we want

[GiacomoPope]

I think potentially the "right" thing to do would be to depreciate all the additional hyperelliptic methods, or move them to the elliptic curve generic if that's better. I don't like that the curve over padic gets a bunch of generic functions just because of a different parent to elliptic curves over other fields

[GiacomoPope]

Using dir(E) on the develop branch and this branch we have:

sage: new = dir(E)
sage: 
sage: for m in new:
....:     if m not in old:
....:         print(m)
....:         
__firstlineno__
__static_attributes__
sage: 
sage: for m in old:
....:     if m not in new:
....:         print(m)
....:         
coleman_integrals_on_basis_hyperelliptic
has_odd_degree_model
jacobian
local_coordinates_at_nonweierstrass
local_coordinates_at_weierstrass
newton_sqrt
odd_degree_model

So it looks like these are the functions we are missing for backwards compatibility at the moment.

[GiacomoPope]

local_coordinates_at_nonweierstrass
local_coordinates_at_weierstrass

Are actually totally critical, so that's a bug. newton_sqrt is actually old redundant code we simply dont need (it's not called anywhere and looks like it should be replaced) The other three could be put in harmlessly

[JohnCremona]

A minor mathematical comment: a curve y^2=F(x) with F of degree 4 over a field K is only an elliptic curve over K if it has K - rational points.

[GiacomoPope]

Thanks @JohnCremona -- I think because of how we have coded the EllipticCurve constructor for this case we will only ever have an odd degree model? You can't make an elliptic curve with a x^4 term in sage directly right?

[GiacomoPope]

I have added all the functions I think need to be included, once I have updated the docstrngs I will make this as ready for review, but if you bhave feedback on the code itself I would be interested.

[vincentmacri]

Okay! Just some cleanup and improvements to error handling left. I'm assuming the remaining TODO items are meant for future work?

[GiacomoPope]

Thanks for the detailed review! Really appreciate the time you've given to this PR

[vincentmacri]

A couple last small things.

[vincentmacri]

Really appreciate the time you've given to this PR

Of course! I do have an ulterior motive though: one of my supervisors and I are hoping to get an undergraduate summer research student to do some work that depends on #39161, lol.

[vincentmacri]

Test failures seem irrelevant. Looks good to me! Thanks for doing this work!

[vincentmacri]

I'll get back to looking at #39161 once this PR is merged into develop.