Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sage.combinat.designs: Modularization fixes, update # needs #35943

Merged
merged 17 commits into from
Jul 30, 2023
Merged

sage.combinat.designs: Modularization fixes, update # needs #35943

merged 17 commits into from
Jul 30, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jul 12, 2023

This is:

📝 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

@mkoeppe mkoeppe self-assigned this Jul 12, 2023
@mkoeppe mkoeppe requested a review from dcoudert July 13, 2023 00:00
dcoudert
dcoudert suggested changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand well, some # optional - ... statements should be # needs ....

Furthermore, for many blocks of tests/examples, we could use sage: # needs ....

src/sage/combinat/cluster_algebra_quiver/interact.py Outdated
@@ -25,8 +25,8 @@ def cluster_interact(self, fig_size=1, circular=True, kind='seed'):

TESTS::

sage: S = ClusterSeed(['A',4]) # optional - sage.graphs
sage: S.interact() # indirect doctest # optional - sage.graphs sage.symbolic
sage: S = ClusterSeed(['A',4]) # optional - sage.graphs sage.modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be # needs sage.graphs sage.modules ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the whole commit making changes outside of sage.combinat.designs was not intended to be here. I'll open a separate PR for those changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's now #35951

src/sage/combinat/cluster_algebra_quiver/mutation_class.py Outdated
@@ -41,11 +42,11 @@ def _principal_part(mat):
EXAMPLES::

sage: from sage.combinat.cluster_algebra_quiver.mutation_class import _principal_part
sage: M = Matrix([[1,2],[3,4],[5,6]]); M
sage: M = Matrix([[1,2],[3,4],[5,6]]); M # optional - sage.modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file too

src/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.py Outdated Show resolved Hide resolved
src/sage/combinat/cluster_algebra_quiver/cluster_seed.py Outdated Show resolved Hide resolved
src/sage/combinat/designs/incidence_structures.py
@@ -929,11 +938,11 @@ def is_regular(self, r=None) -> bool | int:

EXAMPLES::

sage: designs.balanced_incomplete_block_design(7,3).is_regular()
sage: designs.balanced_incomplete_block_design(7,3).is_regular() # needs sage.schemes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply using sage: # needs sage.schemes here for the block ?

Copy link
Member Author

@mkoeppe mkoeppe Jul 13, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sage-fixdoctests script only does this automatically for blocks >= 4 lines
(see #35749 (comment))

src/sage/combinat/designs/incidence_structures.py
@@ -1219,9 +1231,9 @@ def is_berge_cyclic(self):

EXAMPLES::

sage: Hypergraph(5, [[1, 2, 3], [2, 3 ,4]]).is_berge_cyclic()
sage: Hypergraph(5, [[1, 2, 3], [2, 3, 4]]).is_berge_cyclic() # needs sage.modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, we could use sage: # needs sage.modules.

src/sage/combinat/designs/incidence_structures.py
@@ -1431,10 +1445,10 @@ def packing(self, solver=None, verbose=0, *, integrality_tolerance=1e-3):

EXAMPLES::

sage: P = IncidenceStructure([[1,2],[3,4],[2,3]]).packing()
sage: sorted(sorted(b) for b in P)
sage: P = IncidenceStructure([[1,2],[3,4],[2,3]]).packing() # needs sage.numerical.mip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sage: # needs sage.numerical.mip

@mkoeppe
More # optional
20d4211
@mkoeppe
sage.combinat: More # optional
c9b4b06
@mkoeppe
More # optional
55a6eb9
@mkoeppe
sage.combinat: More # optional
dfb21ce
@mkoeppe
src/sage/combinat/designs/block_design.py: Style fixes
cacb502
@mkoeppe
./sage -fixdoctests --distribution 'sagemath-graphs[modules]' --probe…
8441e2c 
… all --only-tags src/sage/combinat/designs/*.{py,pyx}
@mkoeppe
./sage -fixdoctests --only-tags src/sage/combinat/designs/*.{py,pyx}
355bd11
@mkoeppe
./sage -fixdoctests --distribution 'sagemath-graphs' --probe all --on…
ffe78e0 
…ly-tags src/sage/combinat/designs/*.{py,pyx}
@mkoeppe
sage.combinat.designs: Update from sagemath#35095
fdbe7f7
@mkoeppe
sage.combinat.designs: Update file-level directives to use # needs
498feec
@mkoeppe
src/sage/combinat/designs/difference_family.py: Restore lost # random
a7a28fc
@mkoeppe
Do not use block # optional until sagemath#35749 is merged
e7bc5db
@mkoeppe mkoeppe force-pushed the needs_for_sage_combinat_designs branch from 8feae19 to e7bc5db Compare July 13, 2023 16:39
@github-actions
Copy link

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

@dcoudert
Copy link
Contributor

The bot reports the following error:

sage -t --long --random-seed=286735480429121101562228604801325644303 sage/modules/free_module_element.pyx
**********************************************************************
File "sage/modules/free_module_element.pyx", line 941, in sage.modules.free_module_element.FreeModuleElement._giac_init_
Failed example:
    giac(v)+v
Expected:
    [0,2,4,6]
Got:
    Giac crashed -- automatically restarting.
    [sage0,sage1+1,sage2+2,6]

It should not be related

dcoudert
dcoudert approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The error reported by the patchbot concerns giac and is not related to the changes in this PR.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 19, 2023

Thanks a lot!

@vbraun vbraun merged commit 4279c4e into sagemath:develop Jul 30, 2023
13 of 14 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 30, 2023
@mkoeppe mkoeppe mentioned this pull request Aug 6, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees

@mkoeppe mkoeppe

Labels
c: combinatorial designs t: refactoring
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mkoeppe @dcoudert @vbraun