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.

sagemathgh-37722: Remove `CombinatorialClass`       - Fixes sagemath#36133 - Fixes sagemath#33384 - Fixes sagemath#19474 - Fixes sagemath#12913 ### 📝 Checklist  - [ ] The title is concise and informative. - [ ] 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: sagemath#37722 Reported by: Matthias Köppe Reviewer(s): Martin Rubey, Matthias Köppe, Travis Scrimshaw

vbraun · 2024-04-24T22:59:49Z

merge conflict

…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.

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

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

Matthias Koeppe added 12 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

Matthias Koeppe added 2 commits April 28, 2024 12:38

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

18eae69

…__ne__ methods

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

094c106

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

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

Uh oh!

Remove CombinatorialClass #37722

Remove CombinatorialClass #37722

Uh oh!

Conversation

mkoeppe commented Apr 2, 2024

📝 Checklist

⌛ Dependencies

Uh oh!

mkoeppe commented Apr 4, 2024

Uh oh!

mantepse Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mkoeppe Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mkoeppe Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mantepse Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mkoeppe Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mantepse commented Apr 4, 2024

Uh oh!

tscrim left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Apr 4, 2024

Uh oh!

github-actions bot commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tscrim left a comment

Choose a reason for hiding this comment

Uh oh!

mkoeppe commented Apr 8, 2024

Uh oh!

vbraun commented Apr 9, 2024

Uh oh!

mantepse commented Apr 13, 2024

Uh oh!

mkoeppe commented Apr 13, 2024

Uh oh!

vbraun commented Apr 24, 2024

Uh oh!

mantepse commented Apr 28, 2024

Uh oh!

mkoeppe commented Apr 28, 2024

Uh oh!

mkoeppe commented Apr 28, 2024

Uh oh!

mantepse commented Apr 28, 2024

Uh oh!

mkoeppe commented Apr 30, 2024

Uh oh!

Uh oh!

Remove `CombinatorialClass` #37722

Remove `CombinatorialClass` #37722

github-actions bot commented Apr 4, 2024 •

edited

Loading