Document, lint, and fix placement of magic comments in multiline doctests #35181

mkoeppe · 2023-02-23T17:51:59Z

📚 Description

Fixes #33091

tldr:

# optional tags go on the sage: lines.
they don't go on the ....: lines.

📝 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

codecov-commenter · 2023-02-24T06:20:58Z

Codecov Report

Base: 88.60% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (af479f2) compared to base (8f5bbd2).
Patch has no changes to coverable lines.

❗ Current head af479f2 differs from pull request most recent head 1308665. Consider uploading reports for the commit 1308665 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35181      +/-   ##
===========================================
- Coverage    88.60%   88.58%   -0.02%     
===========================================
  Files         2140     2140              
  Lines       397273   397272       -1     
===========================================
- Hits        352004   351937      -67     
- Misses       45269    45335      +66

Impacted Files	Coverage Δ
...omplex_reflection_or_generalized_coxeter_groups.py	`95.04% <ø> (ø)`
...age/categories/finite_complex_reflection_groups.py	`79.12% <ø> (ø)`
src/sage/combinat/abstract_tree.py	`96.04% <ø> (ø)`
src/sage/combinat/crystals/alcove_path.py	`93.72% <ø> (ø)`
src/sage/combinat/designs/bibd.py	`87.83% <ø> (ø)`
src/sage/combinat/designs/incidence_structures.py	`87.98% <ø> (ø)`
src/sage/combinat/designs/orthogonal_arrays.py	`90.42% <ø> (ø)`
...binat/designs/orthogonal_arrays_build_recursive.py	`85.32% <ø> (ø)`
src/sage/combinat/designs/resolvable_bibd.py	`97.16% <ø> (ø)`
...sage/combinat/designs/steiner_quadruple_systems.py	`95.68% <ø> (ø)`
... and 85 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dimpase · 2023-02-24T11:44:17Z

do you have 3a1a03a in the branch? Cause I don't get why you get a test failure. Do you misdetect GAP3 as present?

dimpase · 2023-02-24T11:54:54Z

err, still not enough # optional - gap3 tags.

3a1a03ae2cc (Travis Scrimshaw  2023-01-25 10:43:07 +0900  199)             sage: W = ReflectionGroup((3,1,2))            # optional - gap3
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  200)             sage: data = {w: [w.to_matrix(), w.to_matrix(on_space="dual")] for w in W}  # optional - gap3
3a1a03ae2cc (Travis Scrimshaw  2023-01-25 10:43:07 +0900  201)             sage: for w in W.iteration_tracking_words():  # optional - gap3
3a1a03ae2cc (Travis Scrimshaw  2023-01-25 10:43:07 +0900  202)             ....:     w.reduced_word()                    # optional - gap3
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  203)             ....:     mats = [w.to_matrix(), w.to_matrix(on_space="dual")]  # optional - gap3
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  204)             ....:     mats
14d6f619119 (Travis Scrimshaw  2023-01-26 16:39:54 +0900  205)             ....:     assert data[w] == mats

tobiasdiez · 2023-02-24T14:36:44Z

To reduce these missing optional directives, maybe block level directives would be a handy solution? https://xdoctest.readthedocs.io/en/latest/xdoctest.directive.html

dimpase · 2023-02-24T15:38:55Z

To reduce these missing optional directives, maybe block level directives would be a handy solution? https://xdoctest.readthedocs.io/en/latest/xdoctest.directive.html

looks interesting

dimpase · 2023-02-24T16:57:15Z

I don't know what's up with relint - has anyone tried running what it suggests?

mkoeppe · 2023-02-24T17:53:08Z

I don't know what's up with relint - has anyone tried running what it suggests?

If you're referring to the messages about namespace packages, this is done in #35105, #35106, #35107, which need review

mkoeppe · 2023-02-24T17:55:15Z

still not enough # optional - gap3 tags.

The new linting rule complains because there are too many of these tags, not too few!

mkoeppe · 2023-02-24T18:03:38Z

[...] maybe block level directives would be a handy solution? https://xdoctest.readthedocs.io/en/latest/xdoctest.directive.html

Thanks for the pointer. More expressive/powerful doctest annotations would of course be helpful. Not really for this ticket, which is about getting rid of too many (and badly placed) directives on multi-line doctests. But definitely for the work in #34998 for the modularization effort.

However, block- and function-scope # optional annotations have been discussed many times over the years, and were rejected / discussed to death each time. At least, we were able to get in file-level annotations (top of file # sage.doctest - optional ...).

dimpase · 2023-02-24T18:15:12Z

more tags were needed due to them missing in new develop branch - fixed that

…

On Fri, 24 Feb 2023, 17:55 Matthias Köppe, ***@***.***> wrote: still not enough # optional - gap3 tags. The new linting rule complains because there are *too many* of these tags, not too few! — Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHDWUU5PWG7GV6IP6XTWZDYY5ANCNFSM6AAAAAAVF6EJAY> . You are receiving this because you commented.Message ID: ***@***.***>

mkoeppe · 2023-02-24T18:17:10Z

Fixed what where?

dimpase · 2023-02-24T18:18:35Z

pushed straight to develop
added 2 # optional: gap3 tags

mkoeppe · 2023-02-24T18:22:32Z

These tags that you added in c017a6a are wrong.

mkoeppe · 2023-02-24T18:23:05Z

And CI did not fail with 10.0.beta2 -- https://github.com/sagemath/sage/actions/runs/4258115923

dimpase · 2023-02-24T18:27:35Z

what do you mean? there were errors there

…

On Fri, 24 Feb 2023, 18:22 Matthias Köppe, ***@***.***> wrote: These tags that you added in c017a6a <c017a6a> are wrong. — Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHEX76OGOB7P5URLJ7DWZD37JANCNFSM6AAAAAAVF6EJAY> . You are receiving this because you commented.Message ID: ***@***.***>

dimpase · 2023-02-24T18:28:44Z

On Fri, 24 Feb 2023, 18:23 Matthias Köppe, ***@***.***> wrote: And CI did not fail with 10.0.beta2 -- https://github.com/sagemath/sage/actions/runs/4258115923

yes it did fail, you are probably looking at a different run

…

— Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHEWIKJEMEOG2ZRV3O3WZD4BJANCNFSM6AAAAAAVF6EJAY> . You are receiving this because you commented.Message ID: ***@***.***>

dimpase · 2023-02-24T19:16:31Z

On Fri, 24 Feb 2023, 18:23 Matthias Köppe, @.> wrote: And CI did not fail with 10.0.beta2 -- https://github.com/sagemath/sage/actions/runs/4258115923
yes it did fail, you are probably looking at a different run
…
— Reply to this email directly, view it on GitHub <#35181 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJXYHEWIKJEMEOG2ZRV3O3WZD4BJANCNFSM6AAAAAAVF6EJAY . You are receiving this because you commented.Message ID: @.>

Dude, of course the build was OK, it's build+test that failed.

mkoeppe · 2023-02-24T19:19:17Z

I've pushed the correct fix.

mkoeppe · 2023-02-24T19:20:42Z

Could you please undo the vandalism on the develop branch?

dimpase · 2023-02-24T19:28:56Z

no idea what you are talking about. tags were missing there

mkoeppe · 2023-02-24T19:30:50Z

Read the documentation added here on the branch: https://github.com/sagemath/sage/pull/35181/files#diff-9a1298cd02d22076148519df77cb7b7a99cce074fe4a07c3f4aade37e821705aR1225

dimpase · 2023-02-24T19:33:01Z

Have a look at my commit:
c017a6a

diff --git a/src/sage/combinat/root_system/reflection_group_element.pyx b/src/sage/combinat/root_system/reflection_group_element.pyx
index 02761726799..54553d6f748 100644
--- a/src/sage/combinat/root_system/reflection_group_element.pyx
+++ b/src/sage/combinat/root_system/reflection_group_element.pyx
@@ -201,8 +201,8 @@ cdef class ComplexReflectionGroupElement(PermutationGroupElement):
             sage: for w in W.iteration_tracking_words():  # optional - gap3
             ....:     w.reduced_word()                    # optional - gap3
             ....:     mats = [w.to_matrix(), w.to_matrix(on_space="dual")]  # optional - gap3
-            ....:     mats
-            ....:     assert data[w] == mats
+            ....:     mats                                # optional - gap3
+            ....:     assert data[w] == mats              # optional - gap3
             []
             [
             [1 0]  [1 0]

mkoeppe · 2023-02-24T19:33:38Z

Yes, I've seen it and it is wrong.

dimpase · 2023-02-24T19:34:47Z

after my change, build+test passed. before, it was falling.

mkoeppe · 2023-02-24T19:35:07Z

Read it.

dimpase · 2023-02-24T19:36:59Z

Yes, I've seen it and it is wrong.

the develop branch was wrong.
do you notice tags on the continuation lines?!

If I undo this change, as you insist, it will break again. Feel free to change it as you prefer, though.

mkoeppe · 2023-02-24T19:46:35Z

do you notice tags on the continuation lines?!

Yes, you added them, which is wrong.

Read https://github.com/sagemath/sage/pull/35181/files#diff-9a1298cd02d22076148519df77cb7b7a99cce074fe4a07c3f4aade37e821705aR1225, and you'll understand.

dimpase · 2023-02-24T19:52:09Z

the offending

commit 14d6f6191190904f92728e00989de7ec1b7ff180
Author: Travis Scrimshaw <tcscrims@gmail.com>
Date:   Thu Jan 26 16:39:54 2023 +0900

    Adding some systematic doctests and one detail.

commit 3a1a03ae2cc38d75691418bfda23a703e002139c
Author: Travis Scrimshaw <tcscrims@gmail.com>
Date:   Wed Jan 25 10:43:07 2023 +0900

    Updating __hash__ and updating gap3 doctests to pass.

were added on #34912 - positively reviewed by @stumpc5 (still on trac) who probably didn't notice failing doctests on systems without GAP3 (GAP3 is a good topic for a separate rant...).
And merged by @vbraun in 10.0.beta0.
No idea how it slipped through so far - it's wrong tags placing.

mkoeppe · 2023-02-24T19:54:04Z

You still haven't read it.

dimpase · 2023-02-24T19:54:12Z

do you notice tags on the continuation lines?!

Yes, you added them, which is wrong.

When did I add the tags on the first 2 continuation lines?! They are in @tscrim 's commits!

mkoeppe · 2023-02-24T20:02:26Z

Putting extra # optional on ....: lines does not cause test failures. They are ignored by the doctester. They have no effect. They are just decoration.

Here on the branch I am adding a linter rule that complains about such undisciplined, ineffective decorations, in order to reduce confusion about where tags should be placed.

dimpase · 2023-02-24T20:09:32Z

Putting extra # optional on ....: lines does not cause test failures. They are ignored by the doctester. They have no effect. They are just decoration.

Here on the branch I am adding a linter rule that complains about such undisciplined, ineffective decorations, in order to reduce confusion about where tags should be placed.

I am not talking about linter failures.
I still don't know why build+test failed on these lines. Are you saying you have a linter which runs at test time now?

dimpase · 2023-02-24T20:13:20Z

here is the test run I am talking about: https://github.com/sagemath/sage/actions/runs/4259014013/jobs/7410872930

sage -t --random-seed=271026750805586244091817476045938441266 sage/combinat/root_system/reflection_group_element.pyx
**********************************************************************
File "sage/combinat/root_system/reflection_group_element.pyx", line 200, in sage.combinat.root_system.reflection_group_element.ComplexReflectionGroupElement.to_matrix
Failed example:
    data = {w: [w.to_matrix(), w.to_matrix(on_space="dual")] for w in W}
Exception raised:
    Traceback (most recent call last):
      File "/__w/sage/sage/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/__w/sage/sage/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.root_system.reflection_group_element.ComplexReflectionGroupElement.to_matrix[0]>", line 1, in <module>
        data = {w: [w.to_matrix(), w.to_matrix(on_space="dual")] for w in W}
    NameError: name 'W' is not defined
**********************************************************************
File "sage/combinat/root_system/reflection_group_element.pyx", line 201, in sage.combinat.root_system.reflection_group_element.ComplexReflectionGroupElement.to_matrix
Failed example:
    for w in W.iteration_tracking_words():
        w.reduced_word()
        mats = [w.to_matrix(), w.to_matrix(on_space="dual")]
        mats
        assert data[w] == mats
Exception raised:
    Traceback (most recent call last):
      File "/__w/sage/sage/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/__w/sage/sage/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.root_system.reflection_group_element.ComplexReflectionGroupElement.to_matrix[1]>", line 1, in <module>
        for w in W.iteration_tracking_words():
    NameError: name 'W' is not defined
**********************************************************************
1 item had failures:
   2 of   3 in sage.combinat.root_system.reflection_group_element.ComplexReflectionGroupElement.to_matrix
    [21 tests, 2 failures, 0.10 s]

This is the thing I was fixing in develop.

mkoeppe · 2023-02-24T20:14:39Z

Yes, there was a mistake here on the branch on these lines, which caused a test failure. I have fixed it in 1913358

There was no test failure on develop (10.0.beta2).

mkoeppe · 2023-02-24T20:16:10Z

here is the test run I am talking about: https://github.com/sagemath/sage/actions/runs/4259014013/jobs/7410872930

Yes, this is a run for this branch, not for develop.

There was nothing to fix on develop.

dimpase · 2023-02-24T22:45:14Z

OK, sorry for confusion. I fixed the develop branch, as you might have noticed.

github-actions · 2023-02-24T23:18:09Z

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

mkoeppe · 2023-02-24T23:19:49Z

Thanks! I've merged it already.

tornaria

LGTM

mkoeppe · 2023-02-26T01:46:45Z

Thank you!

vbraun · 2023-03-23T20:10:03Z

Merge conflict, wait for the next beta

There are a few tests in the sage library that span multiple lines and that were intended to be marked as "long time", but were not due to an implementation detail. As it stands, it is the FIRST line of a doctest that needs to be marked "long time", and not the last. This commit fixes a few doctests whose last (but not first) lines were tagged, updating them instead to bear the "# long time" tag on each line. This is overkill, but will save us time in the event that the parser is ever changed.

… go in multiline tests

…t output

… 'physical line'

…ional tags

mkoeppe · 2023-03-27T18:33:09Z

Trivial rebase, back to positive

tornaria approved these changes Feb 26, 2023

View reviewed changes

tornaria added s: positive review and removed s: needs review labels Feb 26, 2023

vbraun removed the s: positive review label Mar 23, 2023

orlitzky and others added 14 commits March 27, 2023 11:27

src/doc/en/developer/coding_basics.rst: Explain where doctest markers…

6ca1f95

… go in multiline tests

src/.relint.yml (multiline_doctest_comment): New

297cc2e

Restore lost # indirect doctest

32c910f

src/doc/pt/tutorial/interfaces.rst: Fix indentation in doctests

f660705

Restore lost # indirect doctest, # known bug

7dd8336

src/doc/en/developer/coding_basics.rst: Expand an example, add doctes…

78608c6

…t output

src/doc/en/developer/coding_basics.rst: Refer to python reference for…

9377f4c

… 'physical line'

src/doc/en/developer/coding_basics.rst: Fix RST markup

7d0b783

src/sage/interfaces/kenzo.py: Fix up doctest markup

32c9b63

Fix magic comments in multiline doctests

116774e

src/sage/geometry/polyhedron/backend_normaliz.py: Fix typo

884cece

src/sage/combinat/root_system/reflection_group_element.pyx: Fix # opt…

2861163

…ional tags

vbraun merged commit 0ae9698 into sagemath:develop Apr 1, 2023

Uh oh!

Document, lint, and fix placement of magic comments in multiline doctests #35181

Document, lint, and fix placement of magic comments in multiline doctests #35181

Uh oh!

Conversation

mkoeppe commented Feb 23, 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 Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

tobiasdiez commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023 via email

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023 via email

Uh oh!

dimpase commented Feb 24, 2023 via email

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

dimpase commented Feb 24, 2023

Uh oh!

mkoeppe commented Feb 23, 2023 •

edited

Loading

codecov-commenter commented Feb 24, 2023 •

edited

Loading

dimpase commented Feb 24, 2023 •

edited

Loading