Remove `CombinatorialClass` #37722

mkoeppe · 2024-04-02T07:13:19Z

Fixes inconsistent depreciation of UnionCombinatorialClass and FilteredCombinatorialClass #36133
Fixes Deprecate global imports of CombinatorialObject, CombinatorialClass, MapCombinatorialClass #33384
Fixes EnumeratedSets.ParentMethods.map should not rely on CombinatorialClass #19474
Fixes Meta-ticket: Deprecate CombinatorialClass in favor of the EnumeratedSet's categories #12913

📝 Checklist

The title is concise and informative.
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 · 2024-04-04T04:55:08Z

Ready for review.

The failure in "Test modularized distributions" is unrelated; it is tracked in #37734.

mantepse · 2024-04-04T06:41:38Z

src/sage/combinat/free_module.py

@@ -1361,7 +1361,7 @@ def __init__(self, modules, **options):
            """
            self._sets = modules
            indices = CartesianProduct_iters(*[module.basis().keys()
-                                               for module in modules]).map(tuple)
+                                               for module in modules]).map(tuple, is_injective=True)


why this change? is_injective is the default, and also what I'd assume that map does.

just for clarity

also what I'd assume that map does.

That's one of the trickier parts in the enumerated sets code. See e.g. #34389

mantepse · 2024-04-04T06:42:31Z

src/sage/combinat/root_system/root_lattice_realizations.py

@@ -1229,7 +1231,7 @@ def negative_roots(self):
            """
            if not self.cartan_type().is_finite():
                raise ValueError("%s is not a finite Cartan type" % self.cartan_type())
-            return self.positive_roots().map(attrcall('__neg__'))
+            return self.positive_roots().map(attrcall('__neg__'), is_injective=True)


why this change? is_injective is the default, and also what I'd assume that map does.

just annotation for clarity as I made my way through the code to fix a failure.

mantepse · 2024-04-04T06:45:16Z

I think that's great!

tscrim

I don't think the ImageSubobject should take the category into account for equality and hashing. It doesn't change what the actual set is. Same for _is_injective. Everything is contained within the self._map, self._inverse, and self._domain_subset.

Other than that, LGTM. We have to pull the trigger at some point on removing CombinatorialClass.

mkoeppe · 2024-04-04T16:58:57Z

I don't think the ImageSubobject should take the category into account for equality and hashing. It doesn't change what the actual set is. Same for _is_injective. Everything is contained within the self._map, self._inverse, and self._domain_subset.

Also self._inverse does not change what the actual set is...

github-actions · 2024-04-04T17:54:49Z

Documentation preview for this PR (built with commit 094c106; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

tscrim

Thank you. LGTM. @mantepse Feel free to revert if you disagree.

mkoeppe · 2024-04-08T02:06:41Z

Thanks!

vbraun · 2024-04-09T20:30:24Z

merge conflict

mantepse · 2024-04-13T07:43:43Z

I'm not sure what happened here, I thought it was a simple merge conflict. Have I overlooked any changes?

mkoeppe · 2024-04-13T16:05:01Z

I set it to "needs review" only to wait for the CI to complete after resolving the merge conflict.
It's all good.

…s by ImageSubobject

…=EnumeratedSets().Finite()

…tive_roots in finite type a *finite* enumerated set; make negative_roots an *injective* image

…__ne__ methods

mantepse · 2024-04-28T22:02:11Z

Hi Matthias! I suppose that you only merged develop, right?

I find it a bit hard ti review if after a merge conflict the whole branch is replaced. I would prefer to see only the difference to the last version.

mkoeppe · 2024-04-28T22:11:53Z

I suppose that you only merged develop, right?

No, I rebased a previous version that was based on 10.4.beta2 (2141959) onto 10.4.beta4.

mkoeppe · 2024-04-28T22:43:50Z

I find it a bit hard ti review if after a merge conflict the whole branch is replaced. I would prefer to see only the difference to the last version.

That's understandable, but ease for the reviewer is only one of several criteria when deciding how to update a branch. Cleanliness of the branch is another one.

I can teach you how to use git more effectively in such situations if you are interested.

mantepse · 2024-04-28T22:54:31Z

I was on my smartphone, so, no git there.

But yes, I'd still be interested to know how to get the "interesting part" of the diff between the branch that was replaced with the current branch.

mkoeppe · 2024-04-30T17:48:14Z

@mantepse A technique that I find useful in such situations is to take the old, reviewed version (here: git fetch upstream 1cdc9cfb1e70a94abf149582c13d588aeaf1b43e && git checkout FETCH_HEAD) and then merge the new release, asking for any merge conflicts to be resolved automatically instead of interactively (using -Xtheirs or -Xours): git merge -Xtheirs upstream/develop. Then the diff with the new version can be inspected.

mkoeppe added c: combinatorics t: refactoring labels Apr 2, 2024

mkoeppe self-assigned this Apr 2, 2024

mkoeppe force-pushed the InfiniteAbstractCombinatorialClass_remove branch 2 times, most recently from d4427d1 to a02f25b Compare April 3, 2024 03:35

mkoeppe added the s: needs review label Apr 4, 2024

mkoeppe requested review from tscrim, mantepse and fchapoton April 4, 2024 04:55

mantepse reviewed Apr 4, 2024

View reviewed changes

tscrim requested changes Apr 4, 2024

View reviewed changes

tscrim added s: positive review and removed s: needs review labels Apr 8, 2024

tscrim approved these changes Apr 8, 2024

View reviewed changes

vbraun added the s: needs work label Apr 9, 2024

mkoeppe force-pushed the InfiniteAbstractCombinatorialClass_remove branch from 2141959 to 1cdc9cf Compare April 12, 2024 23:08

mkoeppe added s: needs review and removed s: positive review s: needs work labels Apr 12, 2024

mkoeppe added s: positive review and removed s: needs review labels Apr 13, 2024

Matthias Koeppe added 14 commits April 28, 2024 12:37

EnumeratedSets.ParentMethods.map: Replace use of MapCombinatorialClas…

0b62b6f

…s by ImageSubobject

src/sage/combinat/combinat.py: Remove CombinatorialClass and subclasses

9e34bf4

Remove CombinatorialClass

ec050a1

src/sage/combinat/species/structure.py: Remove use of CombinatorialClass

9617d44

src/sage/combinat/combinat.py: Remove unused imports

a57171a

src/sage/combinat/species/structure.py (SpeciesWrapper): Use category…

dcf5b3d

…=EnumeratedSets().Finite()

src/sage/combinat/all.py: Restore CombinatorialObject

783df77

src/sage/combinat/root_system/root_lattice_realizations.py: Make posi…

b9fe362

…tive_roots in finite type a *finite* enumerated set; make negative_roots an *injective* image

ImageSubobject: Implement __eq__, __ne__

0ee560c

SpeciesWrapper: Add _repr_ method

29180c0

CartesianProduct_iters: Make hashable if factors are

dcdd8a2

ImageSubobject.__hash__: New

fb84820

src/sage/combinat/species/structure.py (SpeciesWrapper): Add __eq__, …

18eae69

…__ne__ methods

ImageSubobject.{__eq__,__ne__,__hash__}: Only use domain_subset and map

094c106

mkoeppe force-pushed the InfiniteAbstractCombinatorialClass_remove branch from 1cdc9cf to 094c106 Compare April 28, 2024 19:39

mkoeppe added s: needs review and removed s: positive review s: needs work labels Apr 28, 2024

mantepse added s: positive review and removed s: needs review labels Apr 28, 2024

mantepse approved these changes Apr 28, 2024

View reviewed changes

vbraun merged commit 46b7ec2 into sagemath:develop May 2, 2024
18 checks passed

github-actions bot removed the s: positive review label May 2, 2024

mkoeppe added this to the sage-10.4 milestone May 2, 2024

mkoeppe deleted the InfiniteAbstractCombinatorialClass_remove branch May 4, 2024 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `CombinatorialClass` #37722

Remove `CombinatorialClass` #37722

mkoeppe commented Apr 2, 2024

mkoeppe commented Apr 4, 2024

mantepse Apr 4, 2024

mkoeppe Apr 4, 2024

mkoeppe Apr 4, 2024

mantepse Apr 4, 2024

mkoeppe Apr 4, 2024

mantepse commented Apr 4, 2024

tscrim left a comment

mkoeppe commented Apr 4, 2024

github-actions bot commented Apr 4, 2024 •

edited

Loading

tscrim left a comment

mkoeppe commented Apr 8, 2024

vbraun commented Apr 9, 2024

mantepse commented Apr 13, 2024

mkoeppe commented Apr 13, 2024

mantepse commented Apr 28, 2024

mkoeppe commented Apr 28, 2024

mkoeppe commented Apr 28, 2024

mantepse commented Apr 28, 2024

mkoeppe commented Apr 30, 2024

Remove CombinatorialClass #37722

Remove CombinatorialClass #37722

Conversation

mkoeppe commented Apr 2, 2024

📝 Checklist

⌛ Dependencies

mkoeppe commented Apr 4, 2024

mantepse Apr 4, 2024

Choose a reason for hiding this comment

mkoeppe Apr 4, 2024

Choose a reason for hiding this comment

mkoeppe Apr 4, 2024

Choose a reason for hiding this comment

mantepse Apr 4, 2024

Choose a reason for hiding this comment

mkoeppe Apr 4, 2024

Choose a reason for hiding this comment

mantepse commented Apr 4, 2024

tscrim left a comment

Choose a reason for hiding this comment

mkoeppe commented Apr 4, 2024

github-actions bot commented Apr 4, 2024 • edited Loading

tscrim left a comment

Choose a reason for hiding this comment

mkoeppe commented Apr 8, 2024

vbraun commented Apr 9, 2024

mantepse commented Apr 13, 2024

mkoeppe commented Apr 13, 2024

mantepse commented Apr 28, 2024

mkoeppe commented Apr 28, 2024

mkoeppe commented Apr 28, 2024

mantepse commented Apr 28, 2024

mkoeppe commented Apr 30, 2024

Remove `CombinatorialClass` #37722

Remove `CombinatorialClass` #37722

github-actions bot commented Apr 4, 2024 •

edited

Loading