`sage.combinat`: More `# optional` annotations #35742

mkoeppe · 2023-06-07T17:28:11Z

📚 Description

Adding # optional doctest directives (for separate testability of modularized distributions),
and incidental docstring/doctest cosmetics.

Split out from sage.misc.misc, sage.combinat: Modularization fixes #35564
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

…dule level # optional

…ines of excessive length

dcoudert

I think that we should gather some optional tags when these tags are very frequent in a file. Indeed, if a tag is very frequent, it means that we can only test a small fraction of a file without the mentioned dependencies. In such cases, it makes sense to force the entire file to depend on a module.
Furthermore, I don"t understand why adding tag sage.combinat inside a file of the combinat module.

dcoudert · 2023-06-10T12:16:16Z

src/sage/combinat/combinat.py

-            sage: P = L(x)
-            sage: x[0] = 5
-            sage: list(P)
+            sage: L = IntegerListsLex(element_class=Partition)                          # optional - sage.combinat


It's surprising to add optional tag sage.combinat inside files from the combinat module. If it is necessary to add this tag, we should add it directly at the top of all files of the combinat module.

I've started to document the rules for this in #35749 -- see https://deploy-preview-35749--sagemath-tobias.netlify.app/reference/misc/sage/features/sagemath.html#sage.features.sagemath.sage__combinat

In short, some of the most basic modules in sage.combinat are needed already at a very low level. sage.combinat.combinat, .permutations, combinations, etc. will therefore be shipped by the distribution sagemath-categories (see

sage/pkgs/sagemath-categories/MANIFEST.in.m4

Lines 180 to 188 in 7992ac0

include sage/combinat/integer_vector.p*

graft sage/combinat/integer_lists

include sage/combinat/backtrack.p*

include sage/combinat/combinat.p*

include sage/combinat/combinat_cython.p*

include sage/combinat/combination.p*

include sage/combinat/combinatorial_map.p*

include sage/combinat/composition.p*

- in #35095).
These modules then need to mark uses of higher-level modules from sage.combinat with # optional - sage.combinat. (Here it is the use of Partitions that makes it necessary.)

dcoudert · 2023-06-10T12:18:45Z

src/sage/combinat/designs/block_design.py

@@ -318,15 +318,15 @@ def DesarguesianProjectivePlaneDesign(n, point_coordinates=True, check=True):

    EXAMPLES::

-        sage: designs.DesarguesianProjectivePlaneDesign(2)
+        sage: designs.DesarguesianProjectivePlaneDesign(2)                              # optional - sage.rings.finite_rings


Here we could put sage.rings.finite_rings at the top of the file as it seems mandatory for a large portion of the tests.

Indeed from

for a in src/sage/combinat/designs/*.py src/sage/combinat/designs/*.pyx; do echo "$a: $(grep -c '# optional' $a) optional / $(grep -c 'sage: ' $a) doctests" ; done git:sage_combinat_more_optional src/sage/combinat/designs/all.py: 0 optional / 0 doctests src/sage/combinat/designs/bibd.py: 8 optional / 141 doctests src/sage/combinat/designs/block_design.py: 69 optional / 116 doctests src/sage/combinat/designs/covering_design.py: 3 optional / 49 doctests src/sage/combinat/designs/database.py: 97 optional / 395 doctests src/sage/combinat/designs/design_catalog.py: 4 optional / 5 doctests src/sage/combinat/designs/difference_family.py: 121 optional / 362 doctests src/sage/combinat/designs/difference_matrices.py: 0 optional / 20 doctests src/sage/combinat/designs/ext_rep.py: 2 optional / 102 doctests src/sage/combinat/designs/group_divisible_designs.py: 0 optional / 30 doctests src/sage/combinat/designs/incidence_structures.py: 18 optional / 350 doctests src/sage/combinat/designs/latin_squares.py: 0 optional / 43 doctests src/sage/combinat/designs/orthogonal_arrays.py: 0 optional / 187 doctests src/sage/combinat/designs/orthogonal_arrays_build_recursive.py: 0 optional / 73 doctests src/sage/combinat/designs/resolvable_bibd.py: 0 optional / 22 doctests src/sage/combinat/designs/steiner_quadruple_systems.py: 0 optional / 37 doctests src/sage/combinat/designs/twographs.py: 20 optional / 35 doctests src/sage/combinat/designs/designs_pyx.pyx: 34 optional / 99 doctests src/sage/combinat/designs/evenly_distributed_sets.pyx: 7 optional / 50 doctests src/sage/combinat/designs/gen_quadrangles_with_spread.pyx: 0 optional / 52 doctests src/sage/combinat/designs/orthogonal_arrays_find_recursive.pyx: 0 optional / 61 doctests src/sage/combinat/designs/subhypergraph_search.pyx: 0 optional / 12 doctests

... this file is worth a cloer look.

OK, good call. Done in 62d3d48 (+ cosmetics)

dcoudert · 2023-06-10T12:21:52Z

src/sage/combinat/free_module.py

@@ -151,7 +151,7 @@ class CombinatorialFreeModule(UniqueRepresentation, Module, IndexedGenerators):
    The constructed module is in the category of modules with basis
    over the base ring::

-        sage: CombinatorialFreeModule(QQ, Partitions()).category()
+        sage: CombinatorialFreeModule(QQ, Partitions()).category()                      # optional - sage.combinat


again optional tag sage.combinat in a file of the combinat module. Top of the file ?

No, this one definitely should not become a file-level annotation.
Nothing from (the higher level bits of) sage.combinat is used in the implementation of CombinatorialFreeModule, and it will be shipped (as all other free module implementations) in a distribution sagemath-modules; see https://deploy-preview-35749--sagemath-tobias.netlify.app/reference/misc/sage/features/sagemath.html#sage.features.sagemath.sage__modules and

sage/pkgs/sagemath-modules/MANIFEST.in.m4

Line 60 in 7992ac0

include sage/combinat/free_module.py

It is only the use of Partitions again that makes this annotation here necessary. In fact, if it turns out that this file does not have enough test coverage without all the combinatorial tests, then we will actually want to add more tests that are more elementary.

dcoudert · 2023-06-10T12:24:26Z

src/sage/combinat/k_regular_sequence.py

@@ -1,3 +1,4 @@
+# sage.doctest: optional - sage.combinat sage.modules


why not adding sage.symbolic as it is necessary for many doctests ?

I agree. Although some of the doctests can be tested without it, sage.symbolic is needed whenever recurrence equations are used as input, and that seems to be a main functionality of this module.
Moreover, nothing depends on the pair of modules sage.combinat.{k_regular_sequence,recognizable_series}, so there is probably little value in testing a subset of this functionality separately.

Done in 6b97c82.

dcoudert · 2023-06-10T12:26:13Z

src/sage/combinat/ordered_tree.py

            sage: T = OrderedTree( [] )
-            sage: T.to_parallelogram_polyomino()
+            sage: T.to_parallelogram_polyomino()                                        # optional - sage.combinat


top of file ?

I'm shipping the trees from sage.combinat as part of sagemath-graphs; see https://deploy-preview-35749--sagemath-tobias.netlify.app/reference/misc/sage/features/sagemath.html#sage.features.sagemath.sage__graphs
So I want to be able to test these modules without having to pull in sagemath-combinat.
Only the doctests where it connects to other sorts of combinatorics (words, polyominos) are marked # optional

dcoudert · 2023-06-10T12:37:09Z

src/sage/combinat/root_system/root_lattice_realizations.py

                Graphics object consisting of 375 graphics primitives

            The same plot with another alcove walk::

                sage: w2 = [2,1,2,0,2,0,2,1,2,0,1,2,1,2,1,0,1,2,0,2,0,1,2,0,2]
-                sage: p += L.plot_alcove_walk(w2, color="orange")  # long time
+                sage: p += L.plot_alcove_walk(w2, color="orange")  # long time          # optional - sage.plot sage.symbolic


considering the number of times we have sage.plot and sage.symbolic, both could be at the top of the file.

Just went through this file systematically (and made some cosmetic changes in d15a157).

This is fundamental code for root systems; I wouldn't like testing of it to depend on plotting (and symbolics) packages.

One thing that could easily be done though is to move all plotting methods to a separate file and lazy-import from there. Then that file could get a file-level # optional.

mkoeppe · 2023-06-10T16:07:03Z

I think that we should gather some optional tags when these tags are very frequent in a file. Indeed, if a tag is very frequent, it means that we can only test a small fraction of a file without the mentioned dependencies. In such cases, it makes sense to force the entire file to depend on a module.

I agree, and I've done exactly this in many cases already. Sometimes it's obvious from the beginning; but other times I had to go through the file and only realized after adding the tags (I am using editor macros that make this very fast, maybe about 1 second per line) that a file-level annotation would be better. It's quite possible that I have skipped this last step sometimes.

This is something that could be easily fixed using automated tools. Here's just a crude tool that I just pulled from my shell history:
for a in src/sage/coding/*.py src/sage/coding/*.pyx; do echo "$a: $(grep -c '# optional' $a) optional / $(grep -c 'sage: ' $a) doctests" ; done
which I have used to monitor excessive # optionalization.

…r # optional - sage.symbolic

… cosmetics

…s, use file-level # optional - sage.rings.finite_rings, add some # optional - sage.modules

github-actions · 2023-06-10T19:05:12Z

Documentation preview for this PR (built with commit e5fd4e1) is ready! 🎉

dcoudert

This is much better now. I don't like all these # optional - tags, but I don't have any alternative solution to propose. Let's hope one will find a more suitable solution in the future enabling to remove most of these tags.

mkoeppe · 2023-06-11T15:44:54Z

Thanks very much!

tobiasdiez · 2023-06-13T15:06:07Z

Let's wait until we come to a conclusion at https://groups.google.com/g/sage-devel/c/utA0N1it0Eo how to proceed with these optional annotations.

mkoeppe · 2023-06-13T15:46:56Z

Tobias, as I have explained before, the adding these annotations is already documented as our practice.

tobiasdiez · 2023-06-13T17:52:19Z

But clearly these new PRs triggered some discussion on the mailing list. Shouldn't this PR then not wait until the discussion comes to a conclusion?

mkoeppe · 2023-06-13T18:06:04Z

No, I don't think so. I what purpose such a "stop work order" could possibly serve.

There are already 26571 lines of code marked like this in the current beta. Here I'm adding some 4000 more.

Obviously, it is trivial to remove ALL of these annotations using a single shell command if the Sage community comes to the decision that they are not welcome. It makes no difference.

vbraun · 2023-06-13T22:40:43Z

merge conflict

SageMath version 10.1.beta4, Release Date: 2023-06-21

Matthias Koeppe added 21 commits June 7, 2023 10:24

sage.combinat: Add # optional

2fda6a4

sage.combinat: More # optional

429577d

sage.combinat: More # optional

c415d7a

sage.combinat: More # optional

9e9f1f1

sage.combinat: Add # optional, docstring cosmetics

397ebd3

sage.combinat: Add # optional

672a986

sage.combinat: Add # optional - sage.plot

0edead8

sage.combinat: More # optional

0cff99e

sage.combinat: More # optional

f93a484

sage.combinat: More # optional

3ef1b02

src/sage/combinat/k_regular_sequence.py: Fix alignment of # optional

1b54608

src/sage/combinat/{k_regular_sequence,recognizable_series}.py: Add mo…

d516993

…dule level # optional

sage.combinat: More # optional

a2ceb72

Remove stray file

1380e8e

sage.categories: More # optional

a8dbc70

src/sage/combinat/cluster_algebra_quiver/cluster_seed.py: Wrap some l…

3143431

…ines of excessive length

src/sage/combinat/cluster_algebra_quiver/cluster_seed.py: Fix markup

c5568c2

src/sage/combinat/posets: Fix # optional

0f18153

src/sage/combinat/root_system/plot.py: Fix doctest

0b50e1a

src/sage/combinat/cluster_algebra_quiver/cluster_seed.py: Fix markup

2710137

src/sage/combinat/posets/d_complete.py: Fix up doctest

2b0992d

dcoudert suggested changes Jun 10, 2023

View reviewed changes

Matthias Koeppe added 5 commits June 10, 2023 09:18

src/sage/combinat/k_regular_sequence.py: Use file-level annotation fo…

6b97c82

…r # optional - sage.symbolic

src/sage/combinat/root_system/root_lattice_realizations.py: Docstring…

d15a157

… cosmetics

src/sage/combinat/ordered_tree.py: Docstring cosmetics

3178ccb

src/sage/combinat/designs/block_design.py: Doctest/docstring cosmetic…

62d3d48

…s, use file-level # optional - sage.rings.finite_rings, add some # optional - sage.modules

Fix typo of # optional - sage.modules

e5fd4e1

dcoudert approved these changes Jun 11, 2023

View reviewed changes

dcoudert added s: positive review and removed s: needs review labels Jun 11, 2023

tobiasdiez added s: needs info and removed s: positive review labels Jun 13, 2023

vbraun added the s: needs work label Jun 13, 2023

Merge tag '10.1.beta4' into sage_combinat_more_optional

1e24dff

SageMath version 10.1.beta4, Release Date: 2023-06-21

vbraun merged commit ec28487 into sagemath:develop Jul 1, 2023

kwankyu removed the s: positive review label Jul 2, 2023


	include sage/combinat/integer_vector.p*
	graft sage/combinat/integer_lists
	include sage/combinat/backtrack.p*
	include sage/combinat/combinat.p*
	include sage/combinat/combinat_cython.p*
	include sage/combinat/combination.p*
	include sage/combinat/combinatorial_map.p*
	include sage/combinat/composition.p*

		@@ -1,3 +1,4 @@
		# sage.doctest: optional - sage.combinat sage.modules

Uh oh!

sage.combinat: More # optional annotations #35742

sage.combinat: More # optional annotations #35742

Uh oh!

Conversation

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

Uh oh!

📚 Description

📝 Checklist

⌛ Dependencies

Uh oh!

dcoudert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkoeppe Jun 10, 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!

mkoeppe Jun 10, 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!

mkoeppe Jun 10, 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!

mkoeppe commented Jun 10, 2023

Uh oh!

github-actions bot commented Jun 10, 2023

Uh oh!

dcoudert left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Jun 11, 2023

Uh oh!

tobiasdiez commented Jun 13, 2023

Uh oh!

mkoeppe commented Jun 13, 2023

Uh oh!

tobiasdiez commented Jun 13, 2023

Uh oh!

mkoeppe commented Jun 13, 2023

Uh oh!

vbraun commented Jun 13, 2023

Uh oh!

Uh oh!

`sage.combinat`: More `# optional` annotations #35742

`sage.combinat`: More `# optional` annotations #35742

mkoeppe commented Jun 7, 2023 •

edited

Loading

mkoeppe Jun 10, 2023 •

edited

Loading

mkoeppe Jun 10, 2023 •

edited

Loading

mkoeppe Jun 10, 2023 •

edited

Loading