`sage.schemes`: Reformat doctests, add `# optional` annotations #35314

mkoeppe · 2023-03-20T07:39:25Z

📚 Description

Adding doctest tags # optional - sage.rings.finite_rings, ...number_field, ...padics.

While going through the doctests line by line, I also made the following changes:

some coding style fixes in the doctests (such as adding spaces around some operators and after commas, following PEP 8) - improved the readability of them in the HTML format by breaking long lines to avoid having to scroll horizontally
improved indentation of input and output, in particular when morphisms are displayed
made better use of the horizontal space in the doctests of some modules in sage.schemes.toric, which were formatted for a very narrow layout
improved the sphinx markup of some docstrings.

The doctest tags are preparation for being able to test parts of sage.schemes in a modularized setting, in which not all rings are available. See https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#doctest-only-dependencies

Part of:

Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705

📝 Checklist

I have made sure that the title is self-explanatory and the description concisely explains the PR.
I have linked an issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

Depends on sage.rings.function_field: Modularization fixes #35230 (not merged here) for the definition of the feature sage.rings.finite_rings.

The mass edits adding # optional tags were made using the following Emacs macro.

(defun sage-copy-optional-annotation ()
  "In a 'sage: ' line of a docstring, copy '# optional' from a previous line and
advance to the end of the next 'sage: ' line or to the end of the current docstring.
If invoked elsewhere, just advance to the end of the next 'sage: ' line."
  (interactive)
  (if (save-excursion (beginning-of-line)
		      (looking-at " *sage:"))
      (let ((tab-stop-list
	     (list (save-excursion
		     (previous-line)
		     (end-of-line)
		     (re-search-backward "# *optional.*$")
		     (- (match-beginning 0)
			(save-excursion
			  (beginning-of-line) (point)))))))
	(end-of-line)
	(just-one-space)
	(tab-to-tab-stop)
	(insert (match-string-no-properties 0))
	(re-search-forward "sage: \\|\"\"\"")
	(end-of-line))
    (re-search-forward "sage:")
    (end-of-line)))

(with-eval-after-load "python-mode"
  (define-key python-mode-map (kbd "C-M-;") 'sage-copy-optional-annotation))

codecov-commenter · 2023-03-20T17:32:05Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.67 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (ee09844) 87.95%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35314      +/-   ##
===========================================
- Coverage    88.62%   87.95%   -0.67%     
===========================================
  Files         2148     2148              
  Lines       398855   398860       +5     
===========================================
- Hits        353480   350815    -2665     
- Misses       45375    48045    +2670

Impacted Files	Coverage Δ
src/sage/schemes/affine/affine_homset.py	`91.08% <ø> (-1.92%)`	⬇️
src/sage/schemes/affine/affine_morphism.py	`90.00% <ø> (-0.34%)`	⬇️
src/sage/schemes/affine/affine_point.py	`81.25% <ø> (-11.46%)`	⬇️
src/sage/schemes/affine/affine_rational_point.py	`74.69% <ø> (-18.08%)`	⬇️
src/sage/schemes/affine/affine_space.py	`93.81% <ø> (-2.07%)`	⬇️
src/sage/schemes/affine/affine_subscheme.py	`91.86% <ø> (-1.63%)`	⬇️
src/sage/schemes/berkovich/berkovich_cp_element.py	`77.24% <ø> (ø)`
src/sage/schemes/curves/affine_curve.py	`68.75% <ø> (-14.92%)`	⬇️
src/sage/schemes/curves/closed_point.py	`53.62% <ø> (-43.48%)`	⬇️
src/sage/schemes/curves/constructor.py	`81.30% <ø> (-1.63%)`	⬇️
... and 99 more

... and 34 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-03-25T22:54:25Z

Documentation preview for this PR is ready! 🎉
Built with commit: ee09844

src/sage/schemes/affine/affine_homset.py

kwankyu · 2023-03-29T04:23:24Z

src/sage/schemes/affine/affine_morphism.py

@@ -355,7 +347,7 @@ def _fastpolys(self):

            sage: P.<x,y> = AffineSpace(QQ, 2)
            sage: H = Hom(P, P)
-            sage: f = H([x^2+y^2, y^2/(1+x)])
+            sage: f = H([x^2 + y^2, y^2/(1+x)])


needs spaces around the second +.

I'm not adding spaces within nested sums

kwankyu · 2023-03-29T04:24:07Z

src/sage/schemes/affine/affine_morphism.py

@@ -464,58 +456,56 @@ def homogenize(self, n):

            sage: A.<x,y> = AffineSpace(CC, 2)
            sage: H = Hom(A, A)
-            sage: f = H([(x^2-2)/(x*y), y^2-x])
+            sage: f = H([(x^2-2)/(x*y), y^2 - x])


needs spaces

kwankyu · 2023-03-29T04:24:59Z

src/sage/schemes/affine/affine_morphism.py


        ::

            sage: R.<t> = PolynomialRing(ZZ)
            sage: A.<x,y> = AffineSpace(R, 2)
            sage: H = Hom(A, A)
-            sage: f = H([(x^2-2)/y, y^2-x])
+            sage: f = H([(x^2-2)/y, y^2 - x])


needs spaces

kwankyu · 2023-03-29T05:48:07Z

src/sage/schemes/elliptic_curves/constructor.py

@@ -146,7 +146,8 @@ class EllipticCurveFactory(UniqueFactory):

        sage: E = EllipticCurve(CC, [0,0,1,-1,0])
        sage: E
-        Elliptic Curve defined by y^2 + 1.00000000000000*y = x^3 + (-1.00000000000000)*x over Complex Field with 53 bits of precision
+        Elliptic Curve defined by y^2 + 1.00000000000000*y = x^3 + (-1.00000000000000)*x
+        over Complex Field with 53 bits of precision


Sometimes I see

Elliptic Curve ... over ...

Other times

Elliptic Curves ... over ...

Need to be consistent. Here and elsewhere.

I am standardizing on the first format now

Okay for this PR.

But for standardizing how to split a long doctest output and formatting nicely multiline output, I don't know what is right. A related idea is to respect the actual output in the sage terminal. I don't know whether we should or not. This issue perhaps belongs to https://doc.sagemath.org/html/en/developer/coding_basics.html#fine-points-on-styles

I don't think there's a need for setting a policy for developers; to me, anything in the spectrum between the raw actual output (assisted by sage -fixdoctests) and the delicate doctest poetry that I have attempted here is fine. I don't think there is a downside to reformatting the output with the goal of readability / user experience. There is potential for mild disappointment when users try out an example and the output does not look as pretty as it appears in the documentation. But even then the structure-indicating reformatted output guides the user through parsing the complicated raw output. And Sage has various ways to adjust output, using %display latex, %display ascii_art etc.; the difference between documentation and reality may inspire users to explore these facilities (or even improve our _repr_ code).

src/sage/schemes/elliptic_curves/ell_number_field.py

src/sage/schemes/elliptic_curves/height.py

src/sage/schemes/elliptic_curves/hom_composite.py

kwankyu · 2023-03-30T01:21:07Z

src/sage/schemes/berkovich/berkovich_space.py

-        sage: A.<a> = Qp(3).extension(x^3 - 3)
-        sage: B(a)
+        sage: B = Berkovich_Cp_Affine(3)                                                # optional - sage.rings.padics
+        sage: A.<a> = Qp(3).extension(x^3 - 3)                                          # optional - sage.rings.number_field


Extensions of p-adic fields seems to still belong to sage.rings.padics.

Thanks, fixed in ed39a64

kwankyu · 2023-03-30T04:03:57Z

src/sage/schemes/elliptic_curves/hom_frobenius.py

+     from Elliptic Curve defined by y^2 = x^3 + (15*z6^5+5*z6^4+8*z6^3+12*z6^2+11*z6+7)*x
+          over Finite Field in z6 of size 17^6
+       to Elliptic Curve defined by y^2 = x^3 + z6*x
+          over Finite Field in z6 of size 17^6


No optional tags in this file?

Fixed in 5698dfa

kwankyu · 2023-03-30T04:05:25Z

src/sage/schemes/elliptic_curves/isogeny_class.py

-            Isogeny of degree 2 from Elliptic Curve defined by y^2 + x*y + y = x^3 + x^2 - 5*x + 2 over Rational Field to Elliptic Curve defined by y^2 + x*y + y = x^3 + x^2 - 80*x + 242 over Rational Field
+            Isogeny of degree 2
+              from Elliptic Curve defined by y^2 + x*y + y = x^3 + x^2 - 5*x + 2 over Rational Field
+                to Elliptic Curve defined by y^2 + x*y + y = x^3 + x^2 - 80*x + 242 over Rational Field


No optional tags in this file?

Fixed in d5d51b5

kwankyu · 2023-03-30T04:06:38Z

src/sage/schemes/elliptic_curves/isogeny_small_degree.py

+          from Elliptic Curve defined by y^2 = x^3 + 10 over Finite Field in a of size 13^2
+            to Elliptic Curve defined by y^2 = x^3 + (9*a+10)*x + (11*a+12) over Finite Field in a of size 13^2]
+
+        sage: K.<a> = NumberField(x**6 - 320*x**3 - 320)


No optional tags in this file?

Fixed in 289d256

kwankyu · 2023-03-30T06:18:11Z

src/sage/schemes/overview.py

-        sage: P(S)
+        sage: S.<t> = R.quo(x^2 + 5)                                                    # optional - sage.rings.number_field
+        sage: P.<X,Y,Z> = ProjectiveSpace(2, S)                                         # optional - sage.rings.number_field
+        sage: P(S)                                                                      # optional - sage.rings.number_field


S is not really a number field, but a quotient of a polynomial ring.

sage: R.<x> = ZZ[] sage: S.<t> = R.quo(x^2+5) sage: type(S) <class 'sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_domain_with_category'>

Indeed, fixed in 5065211

src/sage/schemes/toric/library.py

kwankyu · 2023-03-30T08:52:00Z

I looked through the changes. They look good to me.

This is a lot of work, and a great improvement of the quality of docstrings in the schemes module.

kwankyu

Thanks.

mkoeppe · 2023-03-31T01:00:06Z

Thanks very much for reviewing this mega-patch!

…padics}; doctest cosmetics

…test output

…test output (ctd.)

…te}.py: Add # optional

…est output

… optional - sage.rings.finite_rings

…nal, reformat doctests

mkoeppe · 2023-04-01T17:42:59Z

Easy rebase.

vbraun · 2023-04-02T00:03:53Z

sage -t --long --warn-long 37.7 --random-seed=123 src/sage/schemes/elliptic_curves/ell_generic.py
**********************************************************************
File "src/sage/schemes/elliptic_curves/ell_generic.py", line 1394, in sage.schemes.elliptic_curves.ell_generic.EllipticCurve_generic.change_ring
Failed example:
    h = F2.hom([a[0]], F4)                                                # optional - sage.rings.finite_rings
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/rings/finite_rings/homset.py", line 100, in __call__
        return FiniteFieldHomomorphism_generic(self, im_gens, base_map=base_map, check=check)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/rings/finite_rings/hom_finite_field.pyx", line 239, in sage.rings.finite_rings.hom_finite_field.FiniteFieldHomomorphism_generic.__init__
        RingHomomorphism_im_gens.__init__(self, parent, im_gens, base_map=base_map, check=check)
      File "sage/rings/morphism.pyx", line 1832, in sage.rings.morphism.RingHomomorphism_im_gens.__init__
        raise ValueError("relations do not all (canonically) map to 0 under map determined by images of generators")
    ValueError: relations do not all (canonically) map to 0 under map determined by images of generators

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/rings/finite_rings/homset.py", line 109, in __call__
        return self._coerce_impl(im_gens)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/rings/finite_rings/homset.py", line 132, in _coerce_impl
        raise TypeError
    TypeError

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.elliptic_curves.ell_generic.EllipticCurve_generic.change_ring[3]>", line 1, in <module>
        h = F2.hom([a[Integer(0)]], F4)                                                # optional - sage.rings.finite_rings
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/structure/parent_gens.pyx", line 300, in sage.structure.parent_gens.ParentWithGens.hom
        return parent.Parent.hom(self, im_gens, codomain, base_map=base_map, category=category, check=check)
      File "sage/structure/parent.pyx", line 1427, in sage.structure.parent.Parent.hom
        return self.Hom(codomain, **Hom_kwds)(im_gens, **kwds)
      File "/home/release/Sage/src/sage/rings/finite_rings/homset.py", line 111, in __call__
        raise TypeError("images do not define a valid homomorphism")
    TypeError: images do not define a valid homomorphism
**********************************************************************
File "src/sage/schemes/elliptic_curves/ell_generic.py", line 1398, in sage.schemes.elliptic_curves.ell_generic.EllipticCurve_generic.change_ring
Failed example:
    E.change_ring(h)                                                      # optional - sage.rings.finite_rings
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.elliptic_curves.ell_generic.EllipticCurve_generic.change_ring[5]>", line 1, in <module>
        E.change_ring(h)                                                      # optional - sage.rings.finite_rings
                      ^
    NameError: name 'h' is not defined
**********************************************************************
1 item had failures:
   2 of   7 in sage.schemes.elliptic_curves.ell_generic.EllipticCurve_generic.change_ring
    [554 tests, 2 failures, 3.71 s]
----------------------------------------------------------------------
sage -t --long --warn-long 37.7 --random-seed=123 src/sage/schemes/elliptic_curves/ell_generic.py  # 2 doctests failed
----------------------------------------------------------------------

### 📚 Description A test is supposed to take < 1s or else be marked # long time. Here we consider slow tests taking >> 10s. When possible we fix or change the test to take less time, otherwise we just mark the test as long time. Occasionally we create a new smaller test and keep the original one as long. After this and #35442 the slowest tests are a few taking ~ 10s. The total time to doctest all goes down from 880 to 806 seconds (using `-tp 8 --all`). ~~NOTE: there's a minor merge conflict with #35314 which I will resolve once that PR is merged.~~ ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. URL: #35443 Reported by: Gonzalo Tornaría Reviewer(s): Gonzalo Tornaría, Matthias Köppe

mkoeppe added c: algebraic geometry t: refactoring labels Mar 20, 2023

mkoeppe self-assigned this Mar 20, 2023

mkoeppe added the s: needs review label Mar 21, 2023

mkoeppe requested a review from yyyyx4 March 22, 2023 17:43

mkoeppe changed the title ~~sage.schemes: Add # optional annotations to doctests~~ sage.schemes: Reformat doctests, add # optional annotations Mar 25, 2023

mkoeppe requested a review from mezzarobba March 25, 2023 21:28

kwankyu reviewed Mar 29, 2023

View reviewed changes

src/sage/schemes/affine/affine_homset.py Outdated Show resolved Hide resolved

kwankyu reviewed Mar 29, 2023

View reviewed changes

src/sage/schemes/elliptic_curves/ell_number_field.py Show resolved Hide resolved

kwankyu reviewed Mar 29, 2023

View reviewed changes

src/sage/schemes/elliptic_curves/height.py Outdated Show resolved Hide resolved

kwankyu reviewed Mar 29, 2023

View reviewed changes

src/sage/schemes/elliptic_curves/hom_composite.py Show resolved Hide resolved

kwankyu reviewed Mar 30, 2023

View reviewed changes

src/sage/schemes/toric/library.py Show resolved Hide resolved

kwankyu approved these changes Mar 30, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Mar 30, 2023

Matthias Koeppe added 2 commits April 1, 2023 10:37

sage.schemes: Add # optional - sage.rings.{number_field,finite_rings,…

c09a90a

…padics}; doctest cosmetics

src/sage/schemes/generic/algebraic_scheme.py: Fix up # optional

e5e29f7

Matthias Koeppe added 18 commits April 1, 2023 10:40

sage.schemes: Yet more cosmetic doctest changes

9c7ae71

sage.schemes: Yet more cosmetic doctest changes (fixup)

3781322

src/sage/schemes/toric/points.py: Fix typo

61a7aeb

sage.schemes: Some more cosmetic doctest changes

7bc90fc

src/sage/schemes/affine/affine_homset.py: Add missing space

fbfbd28

src/sage/schemes/elliptic_curves: Revise indentation of multiline doc…

0f935d0

…test output

src/sage/schemes/curves: Revise indentation of multiline doctest output

20bb0f6

src/sage/schemes/elliptic_curves: Revise indentation of multiline doc…

50cc089

…test output (ctd.)

src/sage/schemes/elliptic_curves/{ell_number_field,height,hom_composi…

9cc0865

…te}.py: Add # optional

src/sage/schemes: Revise indentation of multiline doctest output

e934a4c

src/sage/schemes/elliptic_curves/ell_number_field.py: Fix RST markup

7e48c64

src/sage/schemes: More revisions to the indentation of multiline doct…

7bc2783

…est output

src/sage/schemes/berkovich/berkovich_space.py: Fix up # optional

ad35577

src/sage/schemes/elliptic_curves/hom_frobenius.py: Mark sage.doctest:…

2564510

… optional - sage.rings.finite_rings

src/sage/schemes/elliptic_curves/isogeny_class.py: Mark sage.doctest:…

fcdd127

… optional - sage.rings.finite_rings

src/sage/schemes/elliptic_curves/isogeny_small_degree.py: Add # optio…

7f9cb68

…nal, reformat doctests

src/sage/schemes/overview.py: Fix up # optional

3b34a78

src/sage/schemes/{affine,projective}: Cosmetic changes to doctests

a9588d1

mkoeppe force-pushed the sage_schemes_doctests_optional branch from c63ad60 to a9588d1 Compare April 1, 2023 17:42

vbraun removed the s: positive review label Apr 2, 2023

src/sage/schemes/elliptic_curves/ell_generic.py: Fix up doctest

912e520

mkoeppe added the s: positive review label Apr 2, 2023

vbraun mentioned this pull request Apr 4, 2023

fix E502 in schemes and combinat #35318

Merged

4 tasks

tornaria mentioned this pull request Apr 5, 2023

Fix slow doctests or mark # long time #35443

Merged

2 tasks

vbraun merged commit af4ab84 into sagemath:develop Apr 6, 2023

mkoeppe removed the s: positive review label Apr 7, 2023

mkoeppe added this to the sage-10.0 milestone Apr 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`sage.schemes`: Reformat doctests, add `# optional` annotations #35314

`sage.schemes`: Reformat doctests, add `# optional` annotations #35314

mkoeppe commented Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited

Loading

github-actions bot commented Mar 25, 2023

kwankyu Mar 29, 2023

mkoeppe Mar 29, 2023

kwankyu Mar 29, 2023

kwankyu Mar 29, 2023

kwankyu Mar 29, 2023

mkoeppe Mar 29, 2023

kwankyu Mar 30, 2023

mkoeppe Mar 31, 2023

kwankyu Mar 30, 2023

mkoeppe Mar 30, 2023 •

edited

Loading

kwankyu Mar 30, 2023

mkoeppe Mar 30, 2023

kwankyu Mar 30, 2023

mkoeppe Mar 30, 2023

kwankyu Mar 30, 2023

mkoeppe Mar 30, 2023

kwankyu Mar 30, 2023

mkoeppe Mar 30, 2023

kwankyu commented Mar 30, 2023

kwankyu left a comment

mkoeppe commented Mar 31, 2023

mkoeppe commented Apr 1, 2023

vbraun commented Apr 2, 2023

sage.schemes: Reformat doctests, add # optional annotations #35314

sage.schemes: Reformat doctests, add # optional annotations #35314

Conversation

mkoeppe commented Mar 20, 2023 • edited Loading

📚 Description

📝 Checklist

⌛ Dependencies

codecov-commenter commented Mar 20, 2023 • edited Loading

Codecov Report

github-actions bot commented Mar 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkoeppe Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwankyu commented Mar 30, 2023

kwankyu left a comment

Choose a reason for hiding this comment

mkoeppe commented Mar 31, 2023

mkoeppe commented Apr 1, 2023

vbraun commented Apr 2, 2023

`sage.schemes`: Reformat doctests, add `# optional` annotations #35314

`sage.schemes`: Reformat doctests, add `# optional` annotations #35314

mkoeppe commented Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited

Loading

mkoeppe Mar 30, 2023 •

edited

Loading