Skip to content
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

show explicit formulas in documentation of WeierstrassIsomorphism #34439

Closed
yyyyx4 opened this issue Aug 26, 2022 · 7 comments · Fixed by #34967
Closed

show explicit formulas in documentation of WeierstrassIsomorphism #34439

yyyyx4 opened this issue Aug 26, 2022 · 7 comments · Fixed by #34967

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 26, 2022

Currently, the WeierstrassIsomorphism class doesn't actually specify how the isomorphism given by a tuple (u,r,s,t) is defined.
While this is fairly standard, it can't hurt to make it more explicit in the documentation, hence this very simple patch.

(The related baseWI class does show the coordinate transform, but it's written in the form corresponding more easily to the inverse isomorphism, and the change in curve models is not shown.)

Component: elliptic curves

Author: Lorenz Panny

Issue created by migration from https://trac.sagemath.org/ticket/34439

@yyyyx4 yyyyx4 added this to the sage-9.7 milestone Aug 26, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 31, 2022

Changed commit from bdc3acf to 7e5829a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 31, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7e5829aadd explicit formulas to documentation of WeierstrassIsomorphism

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Sep 27, 2022

comment:4

Sage documentation does not show the docstring of the class __init__ method. Hence your additional documentation reads incomplete. See

https://7e5829a3e3def0e2863ffb3edb2c324be073d1ac--sagemath-tobias.netlify.app/reference/arithmetic_curves/sage/schemes/elliptic_curves/weierstrass_morphism.html#module-sage.schemes.elliptic_curves.weierstrass_morphism

I suggest to move the docstring of __init__ to the class docstring and combine with your docstring (except TESTS blocks). You can use the class ProjectiveCurve

https://github.com/sagemath/sage-prod/blob/develop/src/sage/schemes/curves/projective_curve.py

as a template.

As I skim through elliptic curves modules, I see that many of them need similar edits.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2023

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

11e8992add explicit formulas to documentation of WeierstrassIsomorphism
a8082celift documentation from .__init__() to class and tweak

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2023

Changed commit from 7e5829a to a8082ce

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 16, 2023

comment:6

Thanks for pointing that out. Is it better now?

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 12, 2023

Removed branch from the issue description because it has been replaced by PR #34967

@vbraun vbraun closed this as completed in a10cf50 Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants