Opened 17 months ago

Closed 9 months ago

#32121 closed enhancement (fixed)

Generalize MapCombinatorialClass to ImageSubobject/ImageSet, add method Map.image

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.6
Component: combinatorics Keywords:
Cc: Travis Scrimshaw, Frédéric Chapoton, David Roe, Markus Wageringel Merged in:
Authors: Matthias Koeppe, Travis Scrimshaw Reviewers: Travis Scrimshaw, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 038e354 (Commits, GitHub, GitLab) Commit: 038e354711832119926df56e411759d44f252d80
Dependencies: Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

EnumeratedSets.ParentMethods.map uses MapCombinatorialClass.

map's docstring explains that that class needs to be refactored to use categories.

Part of #12913 (Meta-ticket: Deprecate CombinatorialClass in favor of the EnumeratedSet's categories)

See also:

Change History (51)

comment:1 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:2 Changed 17 months ago by Matthias Köppe

Description: modified (diff)

comment:3 Changed 17 months ago by Matthias Köppe

Cc: David Roe added
Summary: Replace MapCombinatorialClassReplace MapCombinatorialClass, add methods Map.image, Map.pushforward

comment:4 Changed 17 months ago by Matthias Köppe

Branch: u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward

comment:5 Changed 17 months ago by git

Commit: fef2e030ff2a276ae2dea39e26c628a598806c5e

Branch pushed to git repo; I updated commit sha1. New commits:

44f0a31src/sage/combinat/combinat.py: Update copyright according to git blame -w --date=format:%Y src/sage/combinat/combinat.py | sort -k2
fef2e03sage.sets.image_set: New

comment:6 Changed 17 months ago by Matthias Köppe

Dependencies: #32013

comment:7 Changed 17 months ago by git

Commit: fef2e030ff2a276ae2dea39e26c628a598806c5e8e920e9a358241bd330ffaaf832a73c1a24c363a

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

b67092cSet_base.union, intersection, difference, symmetric_difference: Convert to Set if necessary
e144389ConvexSet_base.intersection: Remove abstract method; now inherits Set_base.intersection
bc26d4f{LatticePolytopeClass, ConvexRationalPolyhedralCone._some_elements_: New
e768463Merge #31877
cb93c99RealSet: Inherit from Set_base, Set_boolean_operators, Set_add_sub_operators
3a6f9bdRealSet.symmetric_difference: New
08c52c2Set_base._test_as_set_object: Skip _test_pickling
200b1efMerge tag '9.4.beta4' into t/32013/initialize_a_set_from_a_convexset_base_instance
6a880c0Merge #32013
8e920e9Make MapCombinatorialClass a subclass of ImageSubobject

comment:8 Changed 17 months ago by git

Commit: 8e920e9a358241bd330ffaaf832a73c1a24c363ab57c6a5c19860c18884f4b7e5eba25353cad95d3

Branch pushed to git repo; I updated commit sha1. New commits:

b57c6a5ImageSubobject: Handle is_injective, fix repr

comment:9 Changed 17 months ago by git

Commit: b57c6a5c19860c18884f4b7e5eba25353cad95d35223859004d476310b1c24f06752e1f03ca363be

Branch pushed to git repo; I updated commit sha1. New commits:

5223859ImageSubobject: Fixup finite/infinite

comment:10 Changed 17 months ago by Matthias Köppe

Authors: Matthias Koeppe

Here's an attempt

comment:11 Changed 17 months ago by git

Commit: 5223859004d476310b1c24f06752e1f03ca363be2d0bd77aec3509491c20d1b93129f76d3f73ba97

Branch pushed to git repo; I updated commit sha1. New commits:

2d0bd77Map.image: Fixup

comment:12 Changed 17 months ago by Matthias Köppe

Cc: Markus Wageringel added

comment:13 Changed 17 months ago by git

Commit: 2d0bd77aec3509491c20d1b93129f76d3f73ba977d362129096d2dcdb675e057be69aed83f2fc38a

Branch pushed to git repo; I updated commit sha1. New commits:

7d36212ImageSubobject: Handle CallableSymbolicExpression as map

comment:14 Changed 17 months ago by Matthias Köppe

Summary: Replace MapCombinatorialClass, add methods Map.image, Map.pushforwardGeneralize MapCombinatorialClass to ImageSubobject/ImageSet, add method Map.image

comment:15 Changed 17 months ago by Matthias Köppe

Dependencies: #32013#32013, #32130

comment:16 Changed 17 months ago by git

Commit: 7d362129096d2dcdb675e057be69aed83f2fc38af80d5f4607a158911ff0f8e1d047161a5c12d0f2

Branch pushed to git repo; I updated commit sha1. New commits:

d9cff76Function._sympy_, SympyConverter.__call__: New
17bc9b4Merge #32130
f80d5f4PoorManMap, ImageSubobject: Add _sympy_ method

comment:17 Changed 17 months ago by Matthias Köppe

Help with extending the doctests with examples relevant for combinatorics and algebra would be very welcome.

comment:18 Changed 17 months ago by Matthias Köppe

Status: newneeds_review

comment:19 Changed 17 months ago by git

Commit: f80d5f4607a158911ff0f8e1d047161a5c12d0f284eec2a920ef556b21fe496a734afe6375193564

Branch pushed to git repo; I updated commit sha1. New commits:

63c2cfeConvexSet_base._test_convex_set: Do not test _test_as_set_object here
55240bbMerge #30473
fff2a79src/sage/sets/set.py: Fix docstring markup
e433953Merge #32013
84eec2aMerge tag '9.4.beta5' into t/32121/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward

comment:20 Changed 17 months ago by Travis Scrimshaw

Sorry for being a bit slow to get to this.

This class is not so widely used within the codebase. I can only find:

  • combinat/set_partition_ordered.py: fatter()
  • combinat/root_systems/root_lattice_realizations.py: negative_roots()
  • combinat/composition.py: finer() and fatter()
  • combinat/free_module.py: The indices for the tensor product of CFMs

So if it works fine for these files, then it is a full replacement. These also are fairly extensively tested already I think. Thus, I don't think new tests are necessarily needed.

comment:21 Changed 16 months ago by git

Commit: 84eec2a920ef556b21fe496a734afe63751935649e97ea0b9d328a67ffd4e7cb47780a4b6421d6e7

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

de96639src/sage/combinat/combinat.py: Update copyright according to git blame -w --date=format:%Y src/sage/combinat/combinat.py | sort -k2
4a0d7ddMap.image, sage.sets.image_set: New
61b9b7eMake MapCombinatorialClass a subclass of ImageSubobject
6e0569eImageSubobject: Handle is_injective, fix repr
065a1fcImageSubobject: Fixup finite/infinite
61a8a48ImageSubobject: Handle CallableSymbolicExpression as map
9e97ea0PoorManMap, ImageSubobject: Add _sympy_ method

comment:22 Changed 16 months ago by Matthias Köppe

Dependencies: #32013, #32130

Rebased, partially squashed, on 9.4.rc0

comment:23 Changed 16 months ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewneeds_work

A few trivial failures, but also a more deeper issue:

sage -t --long --random-seed=0 src/sage/categories/finite_dimensional_modules_with_basis.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/combinat/free_module.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/categories/enumerated_sets.py  # 2 doctests failed

Additionally, there are many places that need some doctests. Would you like me to add them?

comment:24 Changed 16 months ago by Matthias Köppe

Any help on the ticket is welcome! I won't be able to work on it before mid August

comment:25 Changed 16 months ago by Matthias Köppe

Milestone: sage-9.4sage-9.5

comment:26 Changed 14 months ago by Travis Scrimshaw

Branch: u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforwardpublic/combinat/image_sets-32121
Commit: 9e97ea0b9d328a67ffd4e7cb47780a4b6421d6e75beba14101f2a6c01d98766d4d1aca5caba5a4c5
Status: needs_workneeds_review

I finally got around to working on this. I have made some tweaks, and I also fixed a few bugs. The biggest was moving the generic image() definition to the category. This was conflicting with the image defined for ModulesWithBasis(). There are still a few things that need doctests, but I don't know how to effectively doctest them (other than with something trivial).


New commits:

8d87ea5Merge branch 'u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward' of git://trac.sagemath.org/sage into public/combinat/image_sets-32121
a7877ceInitial round of fixing issues with ImageSet.
5beba14Fixing remaining things with ImageSet and adding some doctests.

comment:27 Changed 13 months ago by git

Commit: 5beba14101f2a6c01d98766d4d1aca5caba5a4c53197cd345c8449d76632f8d5f820eb7949f026ff

Branch pushed to git repo; I updated commit sha1. New commits:

3197cd3Merge branch 'develop' into public/combinat/image_sets-32121

comment:28 Changed 13 months ago by Matthias Köppe

Authors: Matthias KoeppeMatthias Koeppe, Travis Scrimshaw
Reviewers: Travis ScrimshawTravis Scrimshaw, ...

comment:29 Changed 12 months ago by git

Commit: 3197cd345c8449d76632f8d5f820eb7949f026ffb845bfb437b1027b8ea7b41e8e39f139ec4dd578

Branch pushed to git repo; I updated commit sha1. New commits:

b845bfbMerge branch 'develop' into public/combinat/image_sets-32121

comment:30 Changed 12 months ago by Matthias Köppe

Milestone: sage-9.5sage-9.6

comment:31 Changed 10 months ago by git

Commit: b845bfb437b1027b8ea7b41e8e39f139ec4dd5787c091af52930bfcd80a94d177c61d5f9be8f0ae0

Branch pushed to git repo; I updated commit sha1. New commits:

7c091afMerge tag '9.6.beta1' into t/32121/public/combinat/image_sets-32121

comment:32 Changed 10 months ago by git

Commit: 7c091af52930bfcd80a94d177c61d5f9be8f0ae00d850f1c90df828c69d45570b4ada0b925115312

Branch pushed to git repo; I updated commit sha1. New commits:

0d850f1src/sage/categories/sets_cat.py: Update copyright according to git blame -w --date=format:%Y src/sage/categories/sets_cat.py | sort -k2

comment:33 Changed 10 months ago by git

Commit: 0d850f1c90df828c69d45570b4ada0b9251153120b70dc93b61a5a5e7f40e388d392af80da62dbc4

Branch pushed to git repo; I updated commit sha1. New commits:

0b70dc9src/sage/sets/image_set.py: Small doc edits

comment:34 Changed 10 months ago by git

Commit: 0b70dc93b61a5a5e7f40e388d392af80da62dbc4f8d559aafdf43b1e256210a76bd025751adff8bb

Branch pushed to git repo; I updated commit sha1. New commits:

8688280WIP
f8d559aPoorManMap._sympy_: Add doctest

comment:35 Changed 10 months ago by Matthias Köppe

Reviewers: Travis Scrimshaw, ...Travis Scrimshaw, Matthias Koeppe

comment:36 Changed 10 months ago by git

Commit: f8d559aafdf43b1e256210a76bd025751adff8bb6605ad26fcfcd1fad253c0b4e18f12b2bed8ff18

Branch pushed to git repo; I updated commit sha1. New commits:

6605ad2src/sage/sets/image_set.py: Small doc edit

comment:37 Changed 10 months ago by git

Commit: 6605ad26fcfcd1fad253c0b4e18f12b2bed8ff184bff6c5b1c8cdec7947ffd653a324086bbfd38a2

Branch pushed to git repo; I updated commit sha1. New commits:

4bff6c5MapCombinatorialClass: Remove methods now inherited from ImageSubobject, restore public slots cc, f

comment:38 Changed 10 months ago by git

Commit: 4bff6c5b1c8cdec7947ffd653a324086bbfd38a27c6aacffb39bc16c000ee098a229ec692d9a176b

Branch pushed to git repo; I updated commit sha1. New commits:

7c6aacfEnumeratedSets.map, CombinatorialClass.map, MapCombinatorialClass: New keyword argument is_injective

comment:39 Changed 10 months ago by git

Commit: 7c6aacffb39bc16c000ee098a229ec692d9a176b772970d8693af5e5ef10682a6b9c31dee030fc4b

Branch pushed to git repo; I updated commit sha1. New commits:

772970dsrc/sage/categories/enumerated_sets.py: Remove outdated warning

comment:40 Changed 10 months ago by Matthias Köppe

I think this is ready now

comment:41 Changed 10 months ago by git

Commit: 772970d8693af5e5ef10682a6b9c31dee030fc4bbbc2f02fdcbbc482bb175f3a672aa95f859cefbd

Branch pushed to git repo; I updated commit sha1. New commits:

bbc2f02src/sage/combinat/free_module.py: Update doctest output

comment:42 Changed 10 months ago by Travis Scrimshaw

Thank you. Just a few more little things:

  • Since _star is only called in one place, I think it should just be done inline. This will also save you from having to write a docstring/test for it.
  • lift and retract need doctests.
  • MapCombinatorialClass, _an_element_, and ImageSet are missing periods at the end of their one-line descriptions.
  • In both map() docs:
    -        Use ``is_injective=False`` to get a correct result in this case.
    +        Use ``is_injective=False`` to get a correct result in this case::
     
                 sage: P.map(len, is_injective=False).list()
                 [1, 2, 3, 4]
    
             INPUT:
     
             - ``is_injective`` -- boolean (default: ``True``) whether to assume
    -          that ``f`` is injective.
    +          that ``f`` is injective
    
  • I am not sure how I feel about being so detailed in the code copyright declaration. I understand the legal reason, but it is a bit tedious to maintain (it is already out of date) and might create legal issues with the license (including some people may not be claiming copyright as they did not change that statement). This is probably more towards bikeshedding though...

comment:43 in reply to:  42 Changed 10 months ago by Matthias Köppe

Replying to tscrim:

I am not sure how I feel about being so detailed in the code copyright declaration. I understand the legal reason,

More importantly perhaps, it looks much better if there is year for year activity by several authors. If copyright notices stop in 2006 because the updating work has been neglected, the file and perhaps the whole project looks stale to the casual observer.

it is a bit tedious to maintain (it is already out of date)

It does not have to be perfect. And if it looks updated, people will be motivated to keep it updated.

and might create legal issues with the license (including some people may not be claiming copyright as they did not change that statement).

Copyright is established whether there is intent or not, whenever there is a copyrightable change in a given year.

comment:44 Changed 10 months ago by git

Commit: bbc2f02fdcbbc482bb175f3a672aa95f859cefbdd3a90ee9dcfe765a477641d190aa51bfede7ea23

Branch pushed to git repo; I updated commit sha1. New commits:

d3a90eesrc/sage/sets/image_set.py: Fold ImageSubobject._star into __init__

comment:45 Changed 10 months ago by git

Commit: d3a90ee9dcfe765a477641d190aa51bfede7ea230e9431ce03fe40f069f7afac52f9172052de439e

Branch pushed to git repo; I updated commit sha1. New commits:

2894aa8MapCombinatorialClass: Fix one-line description
b301b59src/sage/sets/image_set.py: Fix one-line descriptions
ed47743src/sage/categories/enumerated_sets.py, src/sage/combinat/combinat.py: Fix doc markup
0e9431cImageSubobject.{lift,retract}: Add doctests

comment:46 Changed 10 months ago by Travis Scrimshaw

Thank you. Green bot => positive review.

comment:47 Changed 10 months ago by Matthias Köppe

Status: needs_reviewpositive_review

Patchbot failures are unrelated.

comment:48 Changed 10 months ago by Volker Braun

Status: positive_reviewneeds_work

This doctest seems to be random

**********************************************************************
File "src/sage/sets/image_set.py", line 275, in sage.sets.image_set.ImageSet
Failed example:
    ImageSet(sos, Set([(3, 4), (3, -4)]))
Expected:
    Image of
     {(3, -4), (3, 4)} by
     The map (x, y) |--> x^2 + y^2 from Vector space of dimension 2 over Symbolic Ring
Got:
    Image of {(3, 4), (3, -4)} by The map (x, y) |--> x^2 + y^2 from Vector space of dimension 2 over Symbolic Ring
**********************************************************************
1 item had failures:
   1 of   9 in sage.sets.image_set.ImageSet
    [52 tests, 1 failure, 0.25 s]
----------------------------------------------------------------------
sage -t --long --random-seed=0 src/sage/sets/image_set.py  # 1 doctest failed
----------------------------------------------------------------------

comment:49 Changed 10 months ago by git

Commit: 0e9431ce03fe40f069f7afac52f9172052de439e038e354711832119926df56e411759d44f252d80

Branch pushed to git repo; I updated commit sha1. New commits:

038e354src/sage/sets/image_set.py: Adjust doctest output to unspecified print order of Sets

comment:50 Changed 10 months ago by Matthias Köppe

Status: needs_workpositive_review

comment:51 Changed 9 months ago by Volker Braun

Branch: public/combinat/image_sets-32121038e354711832119926df56e411759d44f252d80
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.