`sage.misc.misc`, `sage.combinat`: Modularization fixes by mkoeppe · Pull Request #35564 · sagemath/sage

mkoeppe · 2023-04-25T06:02:18Z

📚 Description

move powerset (= subsets) from the problematic sage.misc.misc module to sage.combinat.subset,
likewise we move the implementation of uniq (sage.misc.misc._stable_uniq) and finish the job from Deprecate sage.misc.misc.uniq #27014,
we remove sage.misc.misc.union, deprecated in new OA for n=112,160,176,208,224,352,416,514,544,640,796,896 #16604,
some other fixes to imports.

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

Depends on cython-lint : removing unused imports in polynomials #35672

mkoeppe · 2023-04-27T01:32:23Z

Green. Ready for review

mkoeppe · 2023-05-28T13:22:50Z

merged #35672 to resolve merge conflict

dcoudert

This is really a patch bomb. I don't understand why you mixed modularization and movements of functions from one file to another with deprecation warnings. All these changes are required, but it's too much for a single patch.
I did my best checking all changes. I noted a few issues that could be fixed.

dcoudert · 2023-06-07T16:11:16Z

src/sage/categories/coxeter_groups.py

            for si in s:
                tester.assertEqual(si**2, one)
-            cox_mat = self.coxeter_matrix()
+            try:


I don't understand this change. It hides import errors

These _test_... methods are invoked whenever TestSuite is run on a parent in this category. As of #35095 , most of the root system functionality is shipped as part of sagemath-modules ("because root systems are just sets of vectors"). But parts of the functionality goes through Dynkin diagrams, which are Graphs and therefore depend (at runtime) on parts of sagemath-graphs. The idea is that in the modularized test of sagemath-modules, we can still run the TestSuite and just skip the tests that can't be tested, rather than blaming the implementation class for sagemath-graphs not being installed.

Thanks for the explanation.

dcoudert · 2023-06-07T16:18:13Z

src/sage/combinat/cluster_algebra_quiver/cluster_seed.py

-        - ``return_paths`` -- (default: False) if True, a shortest path of mutations from ``self`` to the given quiver is returned as well.
-        - ``up_to_equivalence`` -- (default: True) if True, only one seed up to simultaneous permutation of rows and columns of the exchange matrix is recorded.
-        - ``sink_source`` -- (default: False) if True, only mutations at sinks and sources are applied.
+        - ``show_depth`` -- (default: ``False``) if ``True``, the current depth of the mutation is shown while computing.


could be formatted in 80 columns mode

I have made the suggested changes (and more) in #35742 (65e2568)

src/sage/combinat/cluster_algebra_quiver/cluster_seed.py

dcoudert · 2023-06-07T16:27:38Z

src/sage/combinat/k_regular_sequence.py

             3: 2-regular sequence -10, -8, -8, -8, -8, -8, -8, -8, -8, -12, ...,
             6: 2-regular sequence 20, 22, 24, 28, 28, 32, 28, 32, 32, 48, ...}
-            sage: shifted_inhomog[2].mu[0].ncols() == 3*inhomogeneities[2].mu[0].ncols()
+            sage: shifted_inhomog[2].mu[0].ncols() == 3*inhomogeneities[2].mu[0].ncols()    # optional - sage.symbolic


Starting from this line, the alignment of # optional... is different than previous lines

Fixed in #35742 (acf16ad)

dcoudert · 2023-06-07T16:34:06Z

src/sage/combinat/posets/hasse_cython_flint.pyx

@@ -0,0 +1,148 @@
+r"""


Shouldn't you add # cython: binding=True ?

answered in #35741 (comment)

mkoeppe · 2023-06-07T16:52:49Z

This is really a patch bomb

Sorry about this, and thanks for taking a look anyway! I'll break it into separate PRs.

… lazy

… to sage.combinat.subset (with deprecation)

… method-local import

…es for imports

…be imported

mkoeppe · 2023-06-07T19:41:22Z

Reduced this PR to what has not been covered by the new PRs

github-actions · 2023-06-07T21:00:00Z

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

dcoudert

LGTM.

dcoudert · 2023-06-10T13:13:49Z

src/sage/categories/coxeter_groups.py

            for si in s:
                tester.assertEqual(si**2, one)
-            cox_mat = self.coxeter_matrix()
+            try:


Thanks for the explanation.

mkoeppe · 2023-06-10T18:56:59Z

Thank you! May I set to "positive review"?

dcoudert · 2023-06-11T11:49:20Z

Thank you! May I set to "positive review"?

done.

gh-35741: `sage.combinat`: Split some Cython modules (modularization fixes)    ### 📚 Description   Some basic modules of `sage.combinat` (`.permutation`, `.combination`, etc.) are needed in the distribution **sagemath-categories** (as of #35095). We make them importable (by splitting a Cython module into several parts and by using lazy imports) and separately testable (using `# optional` doctest directives). Likewise, split the parts of `sage.combinat.posets.hasse_cython` that need the FLINT library out as a separate module (this is for the package **sagemath-graphs** in #35095)  - Split out from #35564 - Part of #29705  ### 📝 Checklist  - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: #35741 Reported by: Matthias Köppe Reviewer(s): David Coudert

gh-35742: `sage.combinat`: More `# optional` annotations    ### 📚 Description  Adding `# optional` doctest directives (for separate testability of modularized distributions), and incidental docstring/doctest cosmetics.   - Split out from #35564 - Part of #29705  ### 📝 Checklist  - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: #35742 Reported by: Matthias Köppe Reviewer(s): David Coudert, Matthias Köppe

dcoudert suggested changes Jun 7, 2023

View reviewed changes

Matthias Koeppe added 13 commits June 7, 2023 12:22

sage.combinat.{combinat,permutation,set_partition}: Make some imports…

9e136c1

… lazy

sage.combinat.subset: Move subsets, powerset here from sage.misc.misc

d7ca298

Update .all imports of subsets, powerset

84ec7c5

Put in PR number for deprecations

c9666c2

Update imports of subsets, powerset

6b96af8

src/sage/arith/misc.py: Remove import cycle

f591ef9

sage.misc.misc: Replace deprecated sorting 'uniq', move stable 'uniq'…

c94c34e

… to sage.combinat.subset (with deprecation)

src/sage/combinat/root_system/coxeter_type.py: Replace lazy import by…

f5e39ad

… method-local import

sage.combinat: Modularization fixes

6a2b1b2

src/sage/combinat/root_system/coxeter_type.py: Fix import

f756d11

src/sage/combinat/designs/incidence_structures.py: Modularization fix…

f5dae8f

…es for imports

src/sage/combinat/interval_posets.py: Don't fail if DyckWords cannot …

0d1bc44

…be imported

sage.combinat: Modularization fixes for imports

fa0c443

dcoudert approved these changes Jun 10, 2023

View reviewed changes

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

tscrim added s: needs info s: positive review and removed s: positive review s: needs info labels Jun 12, 2023

vbraun merged commit 3aa3644 into sagemath:develop Jun 21, 2023

Uh oh!

Comments

Conversation

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

Uh oh!

📚 Description

📝 Checklist

⌛ Dependencies

Uh oh!

mkoeppe commented Apr 27, 2023

Uh oh!

mkoeppe commented May 28, 2023

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

Uh oh!

Uh oh!

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!

mkoeppe commented Jun 7, 2023

Uh oh!

mkoeppe commented Jun 7, 2023

Uh oh!

github-actions bot commented Jun 7, 2023

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 commented Jun 10, 2023

Uh oh!

dcoudert commented Jun 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkoeppe commented Apr 25, 2023 •

edited

Loading

mkoeppe Jun 7, 2023 •

edited

Loading