Closed 172d4fb0-44e5-4192-82f6-dec655f16f77 closed 5 years ago
Author: Anna Somoza
Description changed:
---
+++
@@ -1 +1,3 @@
+As discussed in #22173, the current the design of the hyperelliptic curves classes was not good.
+This changes create the (genus, ring) subclasses dinamically, removing the need for a class/file per case.
Changed keywords from none to days98, hyperelliptic curves
Commit: e4d5db4
Reviewer: Vincent Delecroix
This is a good move.
The logic in the following is weird
fld = map(lambda fnc : fnc(R), [is_FiniteField, is_RationalField, is_pAdicField])
fld_type = fld.index(True)
You would rather do something like
fields = [
("FiniteField", is_FiniteField, HyperellipticCurve_finite_field),
("RationalField", is_RationalField, HyperellipticCurve_rational_field),
("pAdicField", is_pAdicField, HyperellipticCurve_padic_field)]
for name, test, cls in fields:
if test(R):
.... # treat the class
break # the tests are exclusive, no need to run through all cases
Branch pushed to git repo; I updated commit sha1. New commits:
1c35da7 | Modify field-related data treatment |
Agreed, with your approach the code is easier to understand.
Branch pushed to git repo; I updated commit sha1. New commits:
2863afa | Doc now builds |
Similarly, why do you construct a dictionary in the following
+ genus_class = {2:HyperellipticCurve_g2}
+ if g in genus_class.keys():
+ supercls.append(genus_class[g])
This is simpler as
if g == 2:
supercls.append(HyperellipticCurve_g2)
This I did with #22173 in mind, where I will add the class for genus 3 to the dictionary (I am working on it right now). In general, if more genus are added, then using the dictionary leads to a cleaner code than a list of ifs.
I see. What about?
if g == 2:
supercls.append(HyperellipticCurve_g2)
# elif g == 3:
# supercls.append(HyperellipticCurve_g3)
What do you think about removing the inheritance from HyperellipticCurve_generic
in HyperellipticCurve_rational_field
, HyperellipticCurve_padic_field
, etc? All the inheritance is handled by this new function anyway.
In this case, the function would be
cls = [HyperellipticCurve_generic]
...
if len(cls) > 1: # contains more than just HyperellipticCurve_generic
cls = type("HyperellipticCurve_" + "_".join(cls_name), tuple(cls), {})
return cls(PP, f, h, names=names, genus=g)
And last but not the least, you definitely don't to create twice the same class. The first reason for this is that if you construct twice a hyperelliptic curves over rational you want the type to be the same. But the type function will recreate a new class each time (even if it has the same name).
Also, you don't want to create classes that are not necessary. Creating a class for each genus is a waste of time.
Branch pushed to git repo; I updated commit sha1. New commits:
5b5c570 | Modify genus-related data treatment. Store already-created classes for future use. |
Replying to @videlec:
I see. What about?
if g == 2: supercls.append(HyperellipticCurve_g2) # elif g == 3: # supercls.append(HyperellipticCurve_g3)
How about this new version? There I just use the information of the classes that are imported in the module.
Replying to @videlec:
What do you think about removing the inheritance from
HyperellipticCurve_generic
inHyperellipticCurve_rational_field
,HyperellipticCurve_padic_field
, etc? All the inheritance is handled by this new function anyway.In this case, the function would be
cls = [HyperellipticCurve_generic] ... if len(cls) > 1: # contains more than just HyperellipticCurve_generic cls = type("HyperellipticCurve_" + "_".join(cls_name), tuple(cls), {}) return cls(PP, f, h, names=names, genus=g)
The methods in HyperellipticCurve_rational_field
and others use methods from HyperellipticCurve_generic
, so I don't think that it is a good idea.
Replying to @videlec:
And last but not the least, you definitely don't to create twice the same class. The first reason for this is that if you construct twice a hyperelliptic curves over rational you want the type to be the same. But the type function will recreate a new class each time (even if it has the same name).
Also, you don't want to create classes that are not necessary. Creating a class for each genus is a waste of time.
Agreed, I included the functionality and added a test for it.
Replying to @anna-somoza:
Replying to @videlec:
I see. What about?
if g == 2: supercls.append(HyperellipticCurve_g2) # elif g == 3: # supercls.append(HyperellipticCurve_g3)
How about this new version? There I just use the information of the classes that are imported in the module.
This is terrible. Explicit code is to my mind much better. With the current version, a curious person looking at the code would have no clue about how it works.
Otherwise, the caching with the dictionary is ok.
The new version is good as it forces new contributions to follow the same name pattern and only requires to add the new class without changing the source code of the main function.
So I don't think "terrible" is an appropriate adjective! (Unless you have an other way to catch the existing classes?) A list of ifs might just get very long and unpractical.
I do agree that it is not very reader-friendly as it is. This could easily be fixed with more comments/documentation.
Replying to @VivianePons:
The new version is good as it forces new contributions to follow the same name pattern and only requires to add the new class without changing the source code of the main function.
So I don't think "terrible" is an appropriate adjective! (Unless you have an other way to catch the existing classes?) A list of ifs might just get very long and unpractical.
The list has length one and after #22173 it will have length two. I don't understand this objection.
I do agree that it is not very reader-friendly as it is. This could easily be fixed with more comments/documentation.
"Terrible design" is appropriate.
As I said it is unreadable.
The code is sensitive to code injection
sage: import sage.schemes.hyperelliptic_curves.constructor as constructor
sage: constructor.HyperellipticCurve_g3 = int
You might consider this as a super feature, but it is very much error prone.
I understand the objection about security and efficiency. The argument that the list will be of length 2 is not very convincing though: the very purpose of this ticket is to write an architecture which allows for easy extensions.
I would suggest we go back to the initial solution that was proposed: use a dictionary associating the class name to the actual class. I find it much more readable than the list of ifs and easier to extend.
Description changed:
---
+++
@@ -1,3 +1,3 @@
As discussed in #22173, the current the design of the hyperelliptic curves classes was not good.
-This changes create the (genus, ring) subclasses dinamically, removing the need for a class/file per case.
+This changes create the (genus, ring) subclasses dynamically, removing the need for a class/file per case.
Branch pushed to git repo; I updated commit sha1. New commits:
9295319 | Back to using a dictionary to deal with genus-related classes |
Following Viviane's comment, I went back to using a dictionary.
I am not sure of the meaning of the error that the bot gives, I think it comes from the fact that some files where removed and is otherwise green.
You might want to test that inheritance is actually correct in the doctests
sage: R.<t> = PolynomialRing(GF(next_prime(10^9)))
sage: C = HyperellipticCurve(t^5 + t + 1)
sage: import inspect
sage: inspect.getmro(type(C))
(<class 'sage.schemes.hyperelliptic_curves.constructor.HyperellipticCurve_g2_FiniteField_with_category'>,
<class 'sage.schemes.hyperelliptic_curves.constructor.HyperellipticCurve_g2_FiniteField'>,
<class 'sage.schemes.hyperelliptic_curves.hyperelliptic_g2.HyperellipticCurve_g2'>,
<class 'sage.schemes.hyperelliptic_curves.hyperelliptic_finite_field.HyperellipticCurve_finite_field'>,
<class 'sage.schemes.hyperelliptic_curves.hyperelliptic_generic.HyperellipticCurve_generic'>,
...)
Is it better to add a test for every case or with the one you gave it's enough?
Also, while looking into this I realized that I am duplicating the classes with only one superclass.
sage: R.<t> = PolynomialRing(GF(next_prime(10^9)))
sage: C = HyperellipticCurve(t^7 + t + 1)
sage: import inspect
sage: inspect.getmro(type(C))
(<class 'sage.schemes.hyperelliptic_curves.constructor.HyperellipticCurve_FiniteField_with_category'>,
<class 'sage.schemes.hyperelliptic_curves.constructor.HyperellipticCurve_FiniteField'>,
<class 'sage.schemes.hyperelliptic_curves.hyperelliptic_finite_field.HyperellipticCurve_finite_field'>,
<class 'sage.schemes.hyperelliptic_curves.hyperelliptic_generic.HyperellipticCurve_generic'>,
...)
Would it be preferable to initiate the created_classes
dictionary with the classes HyperellipticCurve_g2
, HyperellipticCurve_FiniteField
, etc to avoid that? That could also simplify the last part of the code if HyperellipticCurve_generic
was added too.
Branch pushed to git repo; I updated commit sha1. New commits:
12069c7 | Test added. Dynamic creation of classes is now done with `dynamic_class` function. |
Even nicer.
One last little thing. In the documentation, you need a linebreak after ::
.
Branch pushed to git repo; I updated commit sha1. New commits:
fe002eb | Add missing endline in documentation |
Perfect! Thanks for your patience.
Changed branch from u/annasomoza/redesign_of_the_constructor_for_hyperelliptic_curves to fe002eb
As discussed in #22173, the current the design of the hyperelliptic curves classes was not good.
This changes create the (genus, ring) subclasses dynamically, removing the need for a class/file per case.
CC: @JRSijsling @mstreng @videlec @slel
Component: number theory
Keywords: days98, hyperelliptic curves
Author: Anna Somoza
Branch/Commit:
fe002eb
Reviewer: Vincent Delecroix
Issue created by migration from https://trac.sagemath.org/ticket/27633