Fix hyperelliptic curve dynamic class construction to allow proper method inheritance

[GiacomoPope]

While working on some hyperelliptic code, I realised the dynamic class construction had an issue with inheritance for the genus two classes and certain methods which should have been available were not.

This is discussed in more detail in the issue #37612

I do not know a good fix for this, but I have found a fix which at least allows the functions supposed to be accessed to be accessed.

I have added a small doctest to ensure that the method called for genus two curves is bound to the correct class.

Overview of the fix

The original class was constructed as:

    cls = dynamic_class(class_name,
                        tuple(superclass),
                        cls=HyperellipticCurve_generic,
                        doccls=HyperellipticCurve)

Where superclass is either an empty list, a list with a single child from:

• HyperellipticCurve_finite_field
• HyperellipticCurve_rational_field
• HyperellipticCurve_padic_field
• HyperellipticCurve_g2

Notice that all four of these classes are children of HyperellipticCurve_generic.

Or, in the case of the base field being one of the above AND the curve being genus two this list is of the form:

[HyperellipticCurve_XXX_field, HyperellipticCurve_g2]

In this case, I found that the dynamic class insertion into cls meant that the methods shared by one of the "superclass" (probably should be called base classes?) and cls itself would call the method from cls rather than the superclass and so all specialised functions were unavailable, making the genus two optimisations redundant.

In my new fix, if superclass has a length of zero, I set cls to be HyperellipticCurve_generic, otherwise cls is None.

This seems to work, but I don't know if this is good practice.

Fixes #37612

[GiacomoPope]

Of course another idea would be something like...

    base_cls = None
     if len(superclass) == 0:
         base_cls = HyperellipticCurve_generic

     class_name = "_".join(cls_name)
     cls = dynamic_class(class_name,
                         tuple(superclass),
                         cls=base_cls,
                         doccls=HyperellipticCurve)

Being changed to

     if not superclass:
         superclass.append(HyperellipticCurve_generic)

     class_name = "_".join(cls_name)
     cls = dynamic_class(class_name,
                         tuple(superclass),
                         doccls=HyperellipticCurve)

and simply ensure that if no children of this parent have been used we use this instead?

[GiacomoPope]

After thinking about it a little more, I will write what I think is happening.

When we call

dynamic_class(name_of_class: str, base_classes: tuple[Class], cls: Class, ...)

I believe we take the class supplied in cls and try and add all methods from the base_classes into cls and give this class a new name name_of_class

For the current code, this then means we take

HyperellipticCurve_generic.HyperellipticCurve_g2

As the super class and make something which looks like...

new_class_name = HyperellipticCurve_generic <- HyperellipticCurve_g2

So new_class_name has all the methods of HyperellipticCurve_g2 inserted first into HyperellipticCurve_generic which is then renamed into the new class. However, if there is a method in both HyperellipticCurve_generic and HyperellipticCurve_g2, then the method on HyperellipticCurve_generic is preferred which is why we lose the jacobian() method from HyperellipticCurve_g2 in the above example.

So the question now is what's the right thing to do? As HyperellipticCurve_generic is the parent child of all others, we essentially want to make a dynamic class from all the classes found in construction. We then have

new_class_name(*superclass)

But this will fail when no special classes have been found.

I therefore think

     if not superclass:
         superclass.append(HyperellipticCurve_generic)

     class_name = "_".join(cls_name)
     cls = dynamic_class(class_name,
                         tuple(superclass),
                         doccls=HyperellipticCurve)

Is a reasonable fix. We will either have a tuple of classes whose parent is HyperellipticCurve_generic and when this tuple is empty we simply use the class HyperellipticCurve_generic itself.

[GiacomoPope]

Hi @kwankyu, I hope it's OK to ask for a review on this. I saw you had previously done great work with class refactoring and that you might be a good person to check whether what I did was satisfactory for the long term usage of these constructors.

[GiacomoPope]

Thank you for the comments.

[kwankyu]

Would you rebase to the latest develop?

[GiacomoPope]

Updated and merged.

[kwankyu]

Thanks. LGTM.

[github-actions]

Documentation preview for this PR (built with commit 02aad62; changes) is ready! 🎉