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
Redesign of the constructor for Hyperelliptic curves #27633
Comments
Author: Anna Somoza |
This comment has been minimized.
This comment has been minimized.
Changed keywords from none to days98, hyperelliptic curves |
Commit: |
Reviewer: Vincent Delecroix |
comment:3
This is a good move. The logic in the following is weird
You would rather do something like
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:5
Agreed, with your approach the code is easier to understand. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:7
Similarly, why do you construct a dictionary in the following
This is simpler as
|
comment:8
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
I see. What about?
|
comment:10
What do you think about removing the inheritance from In this case, the function would be
|
comment:11
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:
|
comment:13
Replying to @videlec:
How about this new version? There I just use the information of the classes that are imported in the module. Replying to @videlec:
The methods in Replying to @videlec:
Agreed, I included the functionality and added a test for it. |
comment:14
Replying to @anna-somoza:
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
Otherwise, the caching with the dictionary is ok. |
comment:16
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
Replying to @VivianePons:
The list has length one and after #22173 it will have length two. I don't understand this objection.
"Terrible design" is appropriate.
You might consider this as a super feature, but it is very much error prone.
|
comment:18
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. |
This comment has been minimized.
This comment has been minimized.
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:21
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
You might want to test that inheritance is actually correct in the doctests
|
comment:23
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.
Would it be preferable to initiate the |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:25
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:
|
comment:27
Perfect! Thanks for your patience. |
Changed branch from u/annasomoza/redesign_of_the_constructor_for_hyperelliptic_curves to |
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
The text was updated successfully, but these errors were encountered: