sage.geometry: Update # needs, use block-scoped tags

[mkoeppe]

Cherry-picked from

• Modularization of sagelib: Distributions sagemath-{brial,combinat,eclib,flint,gap,giac,glpk,graphs,groups,homfly,lcalc,libbraiding,libecm,linbox,modules,mpmath,ntl,pari,plot,polyhedra,schemes,singular,standard-no-symbolics,symbolics} #35095
  Part of
• Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705

📝 Checklist

• The title is concise, informative, and self-explanatory.
• 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

[kwankyu]

For instance, sage.geometry.riemannian_manifolds is not included in the distribution sagemath-polyhedra. Then you should not use the distribution to -fixdoctests the files under src/sage/geometry/riemannian_manifolds. Did you care about this?

[mkoeppe]

--if-installed takes care of this automatically

[kwankyu]

OK.

There are a few warnings

File "src/sage/geometry/polyhedron/combinatorial_polyhedron/conversions.pyx", line 38, in sage.geometry.polyhedron.combinatorial_polyhedron.conversions
Warning: Variable 'incidence_matrix_to_bit_rep_of_Vrep' referenced here was set only in doctest marked '# needs sage.combinat'
    face_list = incidence_matrix_to_bit_rep_of_Vrep(P.incidence_matrix())
    [62 tests, 0.40 s]
sage -t --random-seed=66710805610626010854933516970900481595 src/sage/geometry/polyhedron/combinatorial_polyhedron/combinatorial_face.pyx
    [228 tests, 0.82 s]
sage -t --random-seed=66710805610626010854933516970900481595 src/sage/geometry/polyhedron/combinatorial_polyhedron/list_of_faces.pyx
**********************************************************************
File "src/sage/geometry/polyhedron/combinatorial_polyhedron/list_of_faces.pyx", line 46, in sage.geometry.polyhedron.combinatorial_polyhedron.list_of_faces
Warning: Variable 'incidence_matrix_to_bit_rep_of_Vrep' referenced here was set only in doctest marked '# needs sage.combinat'
    face_list = incidence_matrix_to_bit_rep_of_Vrep(P.incidence_matrix())
    [71 tests, 0.36 s]

[kwankyu]

How did they escape from the scrutiny of sage -fixdoctests?

[mkoeppe]

These are false warnings from the doctester. I'll investigate what's going wrong there.
The tags are correct in the file (and the doctest fixer correctly refuses to fix them)

[mkoeppe]

Yes, it's a bug in the doctest parser that affects blocks with continuation lines:

    sage: # needs sage.combinat
    sage: from sage.geometry.polyhedron.combinatorial_polyhedron.conversions \
    ....:         import incidence_matrix_to_bit_rep_of_Vrep
    sage: P = polytopes.associahedron(['A',3])

The block tag is forgotten immediately after the from line.

[mkoeppe]

This is probably from the lines

        # Hack for non-standard backslash line escapes accepted by the current
        # doctest system.

in src/sage/doctest/parsing.py:1092.

I'll try to fix this tomorrow.

[kwankyu]

OK.

[kwankyu]

I investigated this issue.

The "hack" implements the line continuation backslash. See #12415.

So the "hack" removes the line continuation backslash so that the doctester accepts the doctest. In doing this, it inserts a newline character \n.

The added newline character effectively inserts a blank line between two doctest lines, so that the doctester cancels the persistent needs tag. That is why we get the warning message.

Then what should be a cure? It seems that we have two choices.

(1) We can just remove the "hack". Some new doctest failures may appear, but we can just fix them.

(2) We "fix" the "hack" by

--- a/src/sage/doctest/parsing.py
+++ b/src/sage/doctest/parsing.py
@@ -1087,13 +1087,8 @@ class SageDocTestParser(doctest.DocTestParser):
         # doctest system.
         m = backslash_replacer.search(string)
         while m is not None:
-            next_prompt = find_sage_prompt.search(string, m.end())
             g = m.groups()
-            if next_prompt:
-                future = string[m.end():next_prompt.start()] + '\n' + string[next_prompt.start():]
-            else:
-                future = string[m.end():]
-            string = string[:m.start()] + g[0] + "sage:" + g[1] + future
+            string = string[:m.start()] + g[0] + "sage:" + g[1] + string[m.end():]
             m = backslash_replacer.search(string, m.start())
 
         replace_ellipsis = not python_prompt.search(string)

Thus we prevent spurious new lines from being added.

I think we should choose one that has less side effects.

[kwankyu]

By some experiments, it seems that (1) is not an option. Line continuation backslash doesn't work at all without the "hack"...

[mkoeppe]

(2) looks like a good solution to me

[kwankyu]

I agree. There are just three(two) doctests to fix then.

[mkoeppe]

OK, found them and fixed them in #36034.

[kwankyu]

This is a removal of a tag. Is this correct? Was it a misplaced tag or something has changed meanwhile?

[mkoeppe]

This should not have been removed. I'll investigate.

[mkoeppe]

No, the change is actually correct. This example is handled by preprocessing and does not need latte_int.

[kwankyu]

OK.

[kwankyu]

I looked through the changes, which were made by, I guess, sage -fixdoctests automatically. So perhaps they are all correct. Then the purpose of my questions is to assure that sage -fixdoctests is reliable.

If sage -fixdoctests is reliable, then later PRs of the same nature may not need scanning of human eyes, which is mostly tedious.

[mkoeppe]

I looked through the changes, which were made by, I guess, sage -fixdoctests automatically. So perhaps they are all correct. Then the purpose of my questions is to assure that sage -fixdoctests is reliable.

I am creating these PRs by:

• Cherry-picking commits from Modularization of sagelib: Distributions sagemath-{brial,combinat,eclib,flint,gap,giac,glpk,graphs,groups,homfly,lcalc,libbraiding,libecm,linbox,modules,mpmath,ntl,pari,plot,polyhedra,schemes,singular,standard-no-symbolics,symbolics} #35095 (narrowing to a subdirectory). These commits were made either manually to make the tests of modularized distributions work and/or with the help of sage -fixdoctests including probing.
• Running sage -fixdoctests --only-tags
• The final commits with message "Use more block tags" etc. are done by manually adding block tags and then re-running sage -fixdoctests --only-tags.

If sage -fixdoctests is reliable, then later PRs of the same nature may not need scanning of human eyes, which is mostly tedious.

I'd say it's very reliable now -- within its specifications:

• Free-form comments mixed with tags are sometimes removed (also tags such as # abs tol, mixed with # optional tags, are at risk)
• Removal of tags by feature probing sometimes removes too many tags when variables in doctests are being reused. (The result always passes tags, but with the tags removed the doctests can be misleading.)

So eyeballing the changes is still necessary. I do this when I look for opportunities for using more block tags, but it's easy to miss a few bad changes.

[kwankyu]

Thanks for the summary of how you prepared the PR. All reasonable.

I'd say it's very reliable now -- within its specifications:

It seems so to me too.

So eyeballing the changes is still necessary. I do this when I look for opportunities for using more block tags, but it's easy to miss a few bad changes.

I see. I am expecting too much if I think that an automation could go without supervision.

[kwankyu]

LGTM.

[mkoeppe]

Thank you!

[github-actions]

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