Opened 2 years ago
Closed 2 years ago
#27633 closed enhancement (fixed)
Redesign of the constructor for Hyperelliptic curves
Reported by: | annasomoza | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.8 |
Component: | number theory | Keywords: | days98, hyperelliptic curves |
Cc: | sijsling, mstreng, vdelecroix, slelievre | Merged in: | |
Authors: | Anna Somoza | Reviewers: | Vincent Delecroix |
Report Upstream: | N/A | Work issues: | |
Branch: | fe002eb (Commits, GitHub, GitLab) | Commit: | fe002eba9502615124b4d17467a8dce423bb2adf |
Dependencies: | Stopgaps: |
Description (last modified by )
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.
Change History (28)
comment:1 Changed 2 years ago by
- Branch set to u/annasomoza/redesign_of_the_constructor_for_hyperelliptic_curves
comment:2 Changed 2 years ago by
- Cc sijsling mstreng vdelecroix added
- Commit set to e4d5db4ac23d3369e0a4838f0d8a49bd646f088a
- Component changed from PLEASE CHANGE to number theory
- Description modified (diff)
- Keywords days98 hyperelliptic curves added
- Status changed from new to needs_review
- Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 2 years ago by
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to needs_work
comment:4 Changed 2 years ago by
- Commit changed from e4d5db4ac23d3369e0a4838f0d8a49bd646f088a to 1c35da7d8f2230f64fd5eae83629daa0d21bbc4c
Branch pushed to git repo; I updated commit sha1. New commits:
1c35da7 | Modify field-related data treatment
|
comment:5 Changed 2 years ago by
- Status changed from needs_work to needs_review
Agreed, with your approach the code is easier to understand.
comment:6 Changed 2 years ago by
- Commit changed from 1c35da7d8f2230f64fd5eae83629daa0d21bbc4c to 2863afabd251b2b2d57e70295c3457c9532325c0
Branch pushed to git repo; I updated commit sha1. New commits:
2863afa | Doc now builds
|
comment:7 Changed 2 years ago by
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)
comment:8 Changed 2 years ago by
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.
comment:9 Changed 2 years ago by
I see. What about?
if g == 2: supercls.append(HyperellipticCurve_g2) # elif g == 3: # supercls.append(HyperellipticCurve_g3)
comment:10 Changed 2 years ago by
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)
comment:11 follow-up: ↓ 13 Changed 2 years ago by
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.
comment:12 Changed 2 years ago by
- Commit changed from 2863afabd251b2b2d57e70295c3457c9532325c0 to 5b5c570959a5704f168b30faec3132f115482df5
Branch pushed to git repo; I updated commit sha1. New commits:
5b5c570 | Modify genus-related data treatment. Store already-created classes for future use.
|
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 2 years ago by
Replying to vdelecroix:
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 vdelecroix:
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 vdelecroix:
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.
comment:14 in reply to: ↑ 13 Changed 2 years ago by
Replying to annasomoza:
Replying to vdelecroix:
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.
comment:15 Changed 2 years ago by
Otherwise, the caching with the dictionary is ok.
comment:16 follow-up: ↓ 17 Changed 2 years ago by
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.
comment:17 in reply to: ↑ 16 Changed 2 years ago by
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.
- The way it is written goes through several dictionary accesses and a try/except each time a hyperelliptic curve is created. This is a waste of time as you want constructors to be as light as possible.
comment:18 Changed 2 years ago by
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.
comment:19 Changed 2 years ago by
- Cc slelievre added
- Description modified (diff)
comment:20 Changed 2 years ago by
- Commit changed from 5b5c570959a5704f168b30faec3132f115482df5 to 92953194a0bcd415a3c4a3eee55b4e76ed9bd207
Branch pushed to git repo; I updated commit sha1. New commits:
9295319 | Back to using a dictionary to deal with genus-related classes
|
comment:21 Changed 2 years ago by
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.
comment:22 Changed 2 years ago by
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'>, ...)
comment:23 Changed 2 years ago by
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.
comment:24 Changed 2 years ago by
- Commit changed from 92953194a0bcd415a3c4a3eee55b4e76ed9bd207 to 12069c782ba3b2f9dde21b1740fed8df7b45d7d7
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.
|
comment:25 Changed 2 years ago by
Even nicer.
One last little thing. In the documentation, you need a linebreak after ::
.
comment:26 Changed 2 years ago by
- Commit changed from 12069c782ba3b2f9dde21b1740fed8df7b45d7d7 to fe002eba9502615124b4d17467a8dce423bb2adf
Branch pushed to git repo; I updated commit sha1. New commits:
fe002eb | Add missing endline in documentation
|
comment:27 Changed 2 years ago by
- Status changed from needs_review to positive_review
Perfect! Thanks for your patience.
comment:28 Changed 2 years ago by
- Branch changed from u/annasomoza/redesign_of_the_constructor_for_hyperelliptic_curves to fe002eba9502615124b4d17467a8dce423bb2adf
- Resolution set to fixed
- Status changed from positive_review to closed
This is a good move.
The logic in the following is weird
You would rather do something like