Opened 11 months ago
Closed 3 months ago
#32121 closed enhancement (fixed)
Generalize MapCombinatorialClass to ImageSubobject/ImageSet, add method Map.image
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.6 
Component:  combinatorics  Keywords:  
Cc:  tscrim, chapoton, roed, ghmwageringel  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: 
Description (last modified by )
EnumeratedSets.ParentMethods.map
uses MapCombinatorialClass
.
map
's docstring explains that that class needs to be refactored to use categories.
Part of #12913 (Metaticket: Deprecate CombinatorialClass
in favor of the EnumeratedSet
's categories)
See also:
 https://trac.sagemath.org/ticket/31653#comment:1 for a discussion of existing
image
andpushforward
methods of maps.
Change History (51)
comment:1 Changed 11 months ago by
 Description modified (diff)
comment:2 Changed 11 months ago by
 Description modified (diff)
comment:3 Changed 11 months ago by
 Cc roed added
 Summary changed from Replace MapCombinatorialClass to Replace MapCombinatorialClass, add methods Map.image, Map.pushforward
comment:4 Changed 11 months ago by
 Branch set to u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward
comment:5 Changed 11 months ago by
 Commit set to fef2e030ff2a276ae2dea39e26c628a598806c5e
comment:6 Changed 11 months ago by
 Dependencies set to #32013
comment:7 Changed 11 months ago by
 Commit changed from fef2e030ff2a276ae2dea39e26c628a598806c5e to 8e920e9a358241bd330ffaaf832a73c1a24c363a
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
b67092c  Set_base.union, intersection, difference, symmetric_difference: Convert to Set if necessary

e144389  ConvexSet_base.intersection: Remove abstract method; now inherits Set_base.intersection

bc26d4f  {LatticePolytopeClass, ConvexRationalPolyhedralCone._some_elements_: New

e768463  Merge #31877

cb93c99  RealSet: Inherit from Set_base, Set_boolean_operators, Set_add_sub_operators

3a6f9bd  RealSet.symmetric_difference: New

08c52c2  Set_base._test_as_set_object: Skip _test_pickling

200b1ef  Merge tag '9.4.beta4' into t/32013/initialize_a_set_from_a_convexset_base_instance

6a880c0  Merge #32013

8e920e9  Make MapCombinatorialClass a subclass of ImageSubobject

comment:8 Changed 11 months ago by
 Commit changed from 8e920e9a358241bd330ffaaf832a73c1a24c363a to b57c6a5c19860c18884f4b7e5eba25353cad95d3
Branch pushed to git repo; I updated commit sha1. New commits:
b57c6a5  ImageSubobject: Handle is_injective, fix repr

comment:9 Changed 11 months ago by
 Commit changed from b57c6a5c19860c18884f4b7e5eba25353cad95d3 to 5223859004d476310b1c24f06752e1f03ca363be
Branch pushed to git repo; I updated commit sha1. New commits:
5223859  ImageSubobject: Fixup finite/infinite

comment:11 Changed 11 months ago by
 Commit changed from 5223859004d476310b1c24f06752e1f03ca363be to 2d0bd77aec3509491c20d1b93129f76d3f73ba97
Branch pushed to git repo; I updated commit sha1. New commits:
2d0bd77  Map.image: Fixup

comment:12 Changed 11 months ago by
 Cc ghmwageringel added
comment:13 Changed 11 months ago by
 Commit changed from 2d0bd77aec3509491c20d1b93129f76d3f73ba97 to 7d362129096d2dcdb675e057be69aed83f2fc38a
Branch pushed to git repo; I updated commit sha1. New commits:
7d36212  ImageSubobject: Handle CallableSymbolicExpression as map

comment:14 Changed 11 months ago by
 Summary changed from Replace MapCombinatorialClass, add methods Map.image, Map.pushforward to Generalize MapCombinatorialClass to ImageSubobject/ImageSet, add method Map.image
comment:15 Changed 11 months ago by
 Dependencies changed from #32013 to #32013, #32130
comment:16 Changed 11 months ago by
 Commit changed from 7d362129096d2dcdb675e057be69aed83f2fc38a to f80d5f4607a158911ff0f8e1d047161a5c12d0f2
comment:17 Changed 11 months ago by
Help with extending the doctests with examples relevant for combinatorics and algebra would be very welcome.
comment:18 Changed 10 months ago by
 Status changed from new to needs_review
comment:19 Changed 10 months ago by
 Commit changed from f80d5f4607a158911ff0f8e1d047161a5c12d0f2 to 84eec2a920ef556b21fe496a734afe6375193564
Branch pushed to git repo; I updated commit sha1. New commits:
63c2cfe  ConvexSet_base._test_convex_set: Do not test _test_as_set_object here

55240bb  Merge #30473

fff2a79  src/sage/sets/set.py: Fix docstring markup

e433953  Merge #32013

84eec2a  Merge tag '9.4.beta5' into t/32121/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward

comment:20 Changed 10 months ago by
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()
andfatter()
 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 10 months ago by
 Commit changed from 84eec2a920ef556b21fe496a734afe6375193564 to 9e97ea0b9d328a67ffd4e7cb47780a4b6421d6e7
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
de96639  src/sage/combinat/combinat.py: Update copyright according to git blame w date=format:%Y src/sage/combinat/combinat.py  sort k2

4a0d7dd  Map.image, sage.sets.image_set: New

61b9b7e  Make MapCombinatorialClass a subclass of ImageSubobject

6e0569e  ImageSubobject: Handle is_injective, fix repr

065a1fc  ImageSubobject: Fixup finite/infinite

61a8a48  ImageSubobject: Handle CallableSymbolicExpression as map

9e97ea0  PoorManMap, ImageSubobject: Add _sympy_ method

comment:22 Changed 10 months ago by
 Dependencies #32013, #32130 deleted
Rebased, partially squashed, on 9.4.rc0
comment:23 Changed 10 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to needs_work
A few trivial failures, but also a more deeper issue:
sage t long randomseed=0 src/sage/categories/finite_dimensional_modules_with_basis.py # 1 doctest failed sage t long randomseed=0 src/sage/combinat/free_module.py # 1 doctest failed sage t long randomseed=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 10 months ago by
Any help on the ticket is welcome! I won't be able to work on it before mid August
comment:25 Changed 9 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:26 Changed 8 months ago by
 Branch changed from u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward to public/combinat/image_sets32121
 Commit changed from 9e97ea0b9d328a67ffd4e7cb47780a4b6421d6e7 to 5beba14101f2a6c01d98766d4d1aca5caba5a4c5
 Status changed from needs_work to needs_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:
8d87ea5  Merge branch 'u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward' of git://trac.sagemath.org/sage into public/combinat/image_sets32121

a7877ce  Initial round of fixing issues with ImageSet.

5beba14  Fixing remaining things with ImageSet and adding some doctests.

comment:27 Changed 6 months ago by
 Commit changed from 5beba14101f2a6c01d98766d4d1aca5caba5a4c5 to 3197cd345c8449d76632f8d5f820eb7949f026ff
Branch pushed to git repo; I updated commit sha1. New commits:
3197cd3  Merge branch 'develop' into public/combinat/image_sets32121

comment:28 Changed 6 months ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, ...
comment:29 Changed 5 months ago by
 Commit changed from 3197cd345c8449d76632f8d5f820eb7949f026ff to b845bfb437b1027b8ea7b41e8e39f139ec4dd578
Branch pushed to git repo; I updated commit sha1. New commits:
b845bfb  Merge branch 'develop' into public/combinat/image_sets32121

comment:30 Changed 5 months ago by
 Milestone changed from sage9.5 to sage9.6
comment:31 Changed 3 months ago by
 Commit changed from b845bfb437b1027b8ea7b41e8e39f139ec4dd578 to 7c091af52930bfcd80a94d177c61d5f9be8f0ae0
Branch pushed to git repo; I updated commit sha1. New commits:
7c091af  Merge tag '9.6.beta1' into t/32121/public/combinat/image_sets32121

comment:32 Changed 3 months ago by
 Commit changed from 7c091af52930bfcd80a94d177c61d5f9be8f0ae0 to 0d850f1c90df828c69d45570b4ada0b925115312
Branch pushed to git repo; I updated commit sha1. New commits:
0d850f1  src/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 3 months ago by
 Commit changed from 0d850f1c90df828c69d45570b4ada0b925115312 to 0b70dc93b61a5a5e7f40e388d392af80da62dbc4
Branch pushed to git repo; I updated commit sha1. New commits:
0b70dc9  src/sage/sets/image_set.py: Small doc edits

comment:34 Changed 3 months ago by
 Commit changed from 0b70dc93b61a5a5e7f40e388d392af80da62dbc4 to f8d559aafdf43b1e256210a76bd025751adff8bb
comment:35 Changed 3 months ago by
 Reviewers changed from Travis Scrimshaw, ... to Travis Scrimshaw, Matthias Koeppe
comment:36 Changed 3 months ago by
 Commit changed from f8d559aafdf43b1e256210a76bd025751adff8bb to 6605ad26fcfcd1fad253c0b4e18f12b2bed8ff18
Branch pushed to git repo; I updated commit sha1. New commits:
6605ad2  src/sage/sets/image_set.py: Small doc edit

comment:37 Changed 3 months ago by
 Commit changed from 6605ad26fcfcd1fad253c0b4e18f12b2bed8ff18 to 4bff6c5b1c8cdec7947ffd653a324086bbfd38a2
Branch pushed to git repo; I updated commit sha1. New commits:
4bff6c5  MapCombinatorialClass: Remove methods now inherited from ImageSubobject, restore public slots cc, f

comment:38 Changed 3 months ago by
 Commit changed from 4bff6c5b1c8cdec7947ffd653a324086bbfd38a2 to 7c6aacffb39bc16c000ee098a229ec692d9a176b
Branch pushed to git repo; I updated commit sha1. New commits:
7c6aacf  EnumeratedSets.map, CombinatorialClass.map, MapCombinatorialClass: New keyword argument is_injective

comment:39 Changed 3 months ago by
 Commit changed from 7c6aacffb39bc16c000ee098a229ec692d9a176b to 772970d8693af5e5ef10682a6b9c31dee030fc4b
Branch pushed to git repo; I updated commit sha1. New commits:
772970d  src/sage/categories/enumerated_sets.py: Remove outdated warning

comment:40 Changed 3 months ago by
I think this is ready now
comment:41 Changed 3 months ago by
 Commit changed from 772970d8693af5e5ef10682a6b9c31dee030fc4b to bbc2f02fdcbbc482bb175f3a672aa95f859cefbd
Branch pushed to git repo; I updated commit sha1. New commits:
bbc2f02  src/sage/combinat/free_module.py: Update doctest output

comment:42 followup: ↓ 43 Changed 3 months ago by
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
andretract
need doctests.MapCombinatorialClass
,_an_element_
, andImageSet
are missing periods at the end of their oneline 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 3 months ago by
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 3 months ago by
 Commit changed from bbc2f02fdcbbc482bb175f3a672aa95f859cefbd to d3a90ee9dcfe765a477641d190aa51bfede7ea23
Branch pushed to git repo; I updated commit sha1. New commits:
d3a90ee  src/sage/sets/image_set.py: Fold ImageSubobject._star into __init__

comment:45 Changed 3 months ago by
 Commit changed from d3a90ee9dcfe765a477641d190aa51bfede7ea23 to 0e9431ce03fe40f069f7afac52f9172052de439e
Branch pushed to git repo; I updated commit sha1. New commits:
2894aa8  MapCombinatorialClass: Fix oneline description

b301b59  src/sage/sets/image_set.py: Fix oneline descriptions

ed47743  src/sage/categories/enumerated_sets.py, src/sage/combinat/combinat.py: Fix doc markup

0e9431c  ImageSubobject.{lift,retract}: Add doctests

comment:46 Changed 3 months ago by
Thank you. Green bot => positive review.
comment:47 Changed 3 months ago by
 Status changed from needs_review to positive_review
Patchbot failures are unrelated.
comment:48 Changed 3 months ago by
 Status changed from positive_review to needs_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 randomseed=0 src/sage/sets/image_set.py # 1 doctest failed 
comment:49 Changed 3 months ago by
 Commit changed from 0e9431ce03fe40f069f7afac52f9172052de439e to 038e354711832119926df56e411759d44f252d80
Branch pushed to git repo; I updated commit sha1. New commits:
038e354  src/sage/sets/image_set.py: Adjust doctest output to unspecified print order of Sets

comment:50 Changed 3 months ago by
 Status changed from needs_work to positive_review
comment:51 Changed 3 months ago by
 Branch changed from public/combinat/image_sets32121 to 038e354711832119926df56e411759d44f252d80
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
src/sage/combinat/combinat.py: Update copyright according to git blame w date=format:%Y src/sage/combinat/combinat.py  sort k2
sage.sets.image_set: New