Basic functionalities for weighted projective {curves, points, spaces} #40070
Conversation
|
Documentation preview for this PR (built with commit ebe3a94; changes) is ready! 🎉 |
vincentmacri
left a comment
vincentmacri
left a comment
There was a problem hiding this comment.
I've only looked at a few parts of this so I will likely have additional suggestions after you implement these.
Several of your new files are missing a docstring for the file (where you describe what the purpose of the file is and list the main authors who worked on the file). You're also missing the GPL copyright header for some files.
I'm also not seeing your new files at https://doc-pr-40070--sagemath.netlify.app/html/en/. For a PR of this size, I find it a lot easier to read the generated documentation than the docstrings, so I'd like you to fix this before I look at this code further. I think you need to add the new files to some index or something if I remember right.
| # currently, we only support curves in a weighted projective plane | ||
| if n != 2: | ||
| raise NotImplementedError("ambient space has to be a weighted projective plane") | ||
| # currently, we do not perform checks on weighted projective curves |
There was a problem hiding this comment.
Are there any checks that a user would reasonably expect should be performed? If so, document that these checks aren't done (and ideally why they aren't) in the docstring.
|
|
||
| def __init__(self, X, v, check=True): | ||
| """ | ||
| The Python constructor. |
There was a problem hiding this comment.
This is not a complete docstring, I'm assuming this is placeholder text you forgot to replace.
There was a problem hiding this comment.
actually plenty of __init__ docstring uses "The Python constructor" as its description. It's correct (the __init__ method is the Python constructor), it doesn't show up in the user documentation anyway, and it mostly duplicates the class documentation.
generic/morphism.py
159: def __init__(self, parent, codomain=None):
160- """
161- The Python constructor.
162-
163- EXAMPLES::
164-
|
|
||
| return polynomials | ||
|
|
||
| def _latex_(self): |
There was a problem hiding this comment.
For function where it's very clear what the returned type is, why not add a return type annotation? You can do this for all instances of repr and latex in this PR (I noticed you already have it for some).
| def _latex_(self): | |
| def _latex_(self) -> str: |
|
If you're using any particular book that you're basing your implementation/notation/terminology on, please let me know (and include it in the references if you haven't already). Otherwise, the book I've used on this subject is Galbraith's Mathematics of Public Key Cryptography so that's what I'll be looking at as I read this. |
| @@ -0,0 +1 @@ | |||
| # Here so that cython creates the correct module name No newline at end of file | |||
There was a problem hiding this comment.
this file isn't supposed to be committed into git, add it to # Meson temporary files section in .gitignore instead
|
Setting to needs work because of CI failures. I can review this when it is ready. Should this PR be reviewed before #39161? |
|
@grhkm21 Are you still working on this? |
|
Closing in favour of #41170. |
3 participants
In this PR, I implement basic functionalities for weighted projective curves, and subsequently classes those require.
This is required for #39161, as the natural model for hyperelliptic curve is a weighted one.
There are some small functionalities I have to implement, such as
WP.curve, but it's basically done.Most of the code is from the projective case, and I made mainly minor modifications.