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

src/sage/schemes: Doctest cosmetics #37609

Merged
merged 9 commits into from
Mar 31, 2024
Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 14, 2024

Standard reformatting of doctests and their outputs

Split out from #35095

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Copy link
Contributor

@GiacomoPope GiacomoPope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a bunch of suggestions and small comments thoughout. Essentially everything you've done improves the docstrings but I noticed some missing changes. I probably missed even more changes as I only reviewed the diff.

src/sage/schemes/elliptic_curves/kraus.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/period_lattice.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_point.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_point.py Outdated Show resolved Hide resolved
@@ -108,15 +108,15 @@ def c4c6_model(c4, c6, assume_nonsingular=False):

- ``c4``, ``c6`` -- elements of a number field

- ``assume_nonsingular`` (boolean, default False) -- if True,
- ``assume_nonsingular`` (boolean, default ``False``) -- if ``True``,
check for integrality and nosingularity.

OUTPUT:

The elliptic curve with a-invariants [0,0,0,-c4/48,-c6/864], whose
c-invariants are the given c4, c6. If the supplied invariants are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment, should we do c_4 for latex, double backticks for code or leave c4 as c4 in plaintest? This applies to many functions in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably formatting as LaTeX would be good here, but I'm not going to do it in this PR.
I'll be happy to review a follow-up PR that makes further improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I found a moment to take care of some of these markup changes in this file. That's 74c43cd

src/sage/schemes/elliptic_curves/period_lattice.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/period_lattice.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/period_lattice.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/period_lattice.py Outdated Show resolved Hide resolved
src/sage/schemes/hyperelliptic_curves/mestre.py Outdated Show resolved Hide resolved
Matthias Köppe and others added 5 commits March 14, 2024 12:14
Co-authored-by: Giacomo Pope <44242839+GiacomoPope@users.noreply.github.com>
Co-authored-by: Giacomo Pope <44242839+GiacomoPope@users.noreply.github.com>
Co-authored-by: Giacomo Pope <44242839+GiacomoPope@users.noreply.github.com>
Copy link
Contributor

@GiacomoPope GiacomoPope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three last things I noticed. One should fix the failing linter test.

src/sage/schemes/hyperelliptic_curves/mestre.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/ell_point.py Outdated Show resolved Hide resolved
src/sage/schemes/elliptic_curves/kraus.py Outdated Show resolved Hide resolved
Matthias Köppe and others added 3 commits March 17, 2024 10:36
Co-authored-by: Giacomo Pope <44242839+GiacomoPope@users.noreply.github.com>
Co-authored-by: Giacomo Pope <44242839+GiacomoPope@users.noreply.github.com>
Co-authored-by: Giacomo Pope <44242839+GiacomoPope@users.noreply.github.com>
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 17, 2024

Thanks for the edits! Good to go now?

Copy link

Documentation preview for this PR (built with commit 86143e8; changes) is ready! 🎉

@GiacomoPope
Copy link
Contributor

Yep! LGTM. Thanks so much to taking the time to clean these things up.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 17, 2024

Thanks!

@vbraun vbraun merged commit ef7a598 into sagemath:develop Mar 31, 2024
15 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants