sage.misc.misc, sage.combinat: Modularization fixes
#35564
Conversation
|
Green. Ready for review |
mkoeppe
force-pushed
the
sage_combinat_modularization
branch
from
8f24e58 to
9fe1cd6
Compare
|
merged #35672 to resolve merge conflict |
There was a problem hiding this comment.
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.
| @@ -1327,7 +1327,10 @@ def _test_coxeter_relations(self, **options): | |||
| one = self.one() | |||
| for si in s: | |||
| tester.assertEqual(si**2, one) | |||
| cox_mat = self.coxeter_matrix() | |||
| try: | |||
There was a problem hiding this comment.
I don't understand this change. It hides import errors
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for the explanation.
| - ``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. |
There was a problem hiding this comment.
could be formatted in 80 columns mode
| {2: 2-regular sequence 8, 8, 8, 12, 12, 16, 12, 16, 12, 24, ..., | ||
| 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 |
There was a problem hiding this comment.
Starting from this line, the alignment of # optional... is different than previous lines
| @@ -0,0 +1,148 @@ | |||
| r""" | |||
There was a problem hiding this comment.
Shouldn't you add # cython: binding=True ?
There was a problem hiding this comment.
answered in #35741 (comment)
Sorry about this, and thanks for taking a look anyway! I'll break it into separate PRs. |
… to sage.combinat.subset (with deprecation)
… method-local import
mkoeppe
force-pushed
the
sage_combinat_modularization
branch
from
87a04b8 to
fa0c443
Compare
|
Reduced this PR to what has not been covered by the new PRs |
|
Documentation preview for this PR (built with commit fa0c443) is ready! 🎉 |
mkoeppe
changed the title
sage.combinat: Modularization fixessage.misc.misc, sage.combinat: Modularization fixes
| @@ -1327,7 +1327,10 @@ def _test_coxeter_relations(self, **options): | |||
| one = self.one() | |||
| for si in s: | |||
| tester.assertEqual(si**2, one) | |||
| cox_mat = self.coxeter_matrix() | |||
| try: | |||
There was a problem hiding this comment.
Thanks for the explanation.
|
Thank you! May I set to "positive review"? |
done. |
tscrim
added
s: needs info
s: positive review
and removed
s: positive review
s: needs info
labels
…fixes)
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->
### 📚 Description
<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
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)
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
- Split out from #35564
- Part of #29705
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->
- [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
<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: #35741
Reported by: Matthias Köppe
Reviewer(s): David Coudert
<!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes #12345", use "Add a new method to multiply two integers" --> ### 📚 Description <!-- Describe your changes here in detail. --> Adding `# optional` doctest directives (for separate testability of modularized distributions), and incidental docstring/doctest cosmetics. <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> - Split out from #35564 - Part of #29705 <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [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 <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #35742 Reported by: Matthias Köppe Reviewer(s): David Coudert, Matthias Köppe
📚 Description
powerset(=subsets) from the problematicsage.misc.miscmodule tosage.combinat.subset,uniq(sage.misc.misc._stable_uniq) and finish the job from Deprecate sage.misc.misc.uniq #27014,sage.misc.misc.union, deprecated in new OA for n=112,160,176,208,224,352,416,514,544,640,796,896 #16604,Part of:
📝 Checklist
⌛ Dependencies