`sage.schemes`: Reformat doctests, add `# optional` annotations by mkoeppe · Pull Request #35314 · sagemath/sage

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

            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

            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: 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

        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
----------------------------------------------------------------------

gh-35443: Fix slow doctests or mark # long time ### 📚 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

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 9 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

sage.schemes: More # optional

5e7d3f0

sage.schemes.curves: Add # optional

4f53f6e

sage.schemes: Fix up # optional

7f6ea46

sage.schemes: More cosmetic changes to doctests

b778689

sage.schemes: More # optional and cosmetic doctest changes

919ed50

sage.schemes: More # optional and cosmetic doctest changes (fixup)

47bca76

sage.schemes: More cosmetic doctest changes

d97da6f

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

sage.schemes: More # optional and cosmetic doctest changes

6d21c17

sage.schemes: More # optional and cosmetic doctest changes (fixup)

fdb625e

sage.schemes: More cosmetic doctest changes

a658137

sage.schemes: Even more cosmetic doctest changes

2507c8f

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

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

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

912e520

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

Uh oh!

Comments

Conversation

mkoeppe commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Description

📝 Checklist

⌛ Dependencies

Uh oh!

codecov-commenter commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkoeppe Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwankyu commented Mar 30, 2023

Uh oh!

kwankyu left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Mar 31, 2023

Uh oh!

mkoeppe commented Apr 1, 2023

Uh oh!

vbraun commented Apr 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkoeppe commented Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited

Loading

mkoeppe Mar 30, 2023 •

edited

Loading