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:  sage9.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: 
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 17 months ago by
Description:  modified (diff) 

comment:2 Changed 17 months ago by
Description:  modified (diff) 

comment:3 Changed 17 months ago by
Cc:  David Roe added 

Summary:  Replace MapCombinatorialClass → Replace MapCombinatorialClass, add methods Map.image, Map.pushforward 
comment:4 Changed 17 months ago by
Branch:  → u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward 

comment:5 Changed 17 months ago by
Commit:  → fef2e030ff2a276ae2dea39e26c628a598806c5e 

comment:6 Changed 17 months ago by
Dependencies:  → #32013 

comment:7 Changed 17 months ago by
Commit:  fef2e030ff2a276ae2dea39e26c628a598806c5e → 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 17 months ago by
Commit:  8e920e9a358241bd330ffaaf832a73c1a24c363a → b57c6a5c19860c18884f4b7e5eba25353cad95d3 

Branch pushed to git repo; I updated commit sha1. New commits:
b57c6a5  ImageSubobject: Handle is_injective, fix repr

comment:9 Changed 17 months ago by
Commit:  b57c6a5c19860c18884f4b7e5eba25353cad95d3 → 5223859004d476310b1c24f06752e1f03ca363be 

Branch pushed to git repo; I updated commit sha1. New commits:
5223859  ImageSubobject: Fixup finite/infinite

comment:11 Changed 17 months ago by
Commit:  5223859004d476310b1c24f06752e1f03ca363be → 2d0bd77aec3509491c20d1b93129f76d3f73ba97 

Branch pushed to git repo; I updated commit sha1. New commits:
2d0bd77  Map.image: Fixup

comment:12 Changed 17 months ago by
Cc:  Markus Wageringel added 

comment:13 Changed 17 months ago by
Commit:  2d0bd77aec3509491c20d1b93129f76d3f73ba97 → 7d362129096d2dcdb675e057be69aed83f2fc38a 

Branch pushed to git repo; I updated commit sha1. New commits:
7d36212  ImageSubobject: Handle CallableSymbolicExpression as map

comment:14 Changed 17 months ago by
Summary:  Replace MapCombinatorialClass, add methods Map.image, Map.pushforward → Generalize MapCombinatorialClass to ImageSubobject/ImageSet, add method Map.image 

comment:15 Changed 17 months ago by
Dependencies:  #32013 → #32013, #32130 

comment:16 Changed 17 months ago by
Commit:  7d362129096d2dcdb675e057be69aed83f2fc38a → f80d5f4607a158911ff0f8e1d047161a5c12d0f2 

comment:17 Changed 17 months ago by
Help with extending the doctests with examples relevant for combinatorics and algebra would be very welcome.
comment:18 Changed 17 months ago by
Status:  new → needs_review 

comment:19 Changed 17 months ago by
Commit:  f80d5f4607a158911ff0f8e1d047161a5c12d0f2 → 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 17 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 16 months ago by
Commit:  84eec2a920ef556b21fe496a734afe6375193564 → 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 16 months ago by
Dependencies:  #32013, #32130 

Rebased, partially squashed, on 9.4.rc0
comment:23 Changed 16 months ago by
Reviewers:  → Travis Scrimshaw 

Status:  needs_review → 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 16 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 16 months ago by
Milestone:  sage9.4 → sage9.5 

comment:26 Changed 14 months ago by
Branch:  u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward → public/combinat/image_sets32121 

Commit:  9e97ea0b9d328a67ffd4e7cb47780a4b6421d6e7 → 5beba14101f2a6c01d98766d4d1aca5caba5a4c5 
Status:  needs_work → 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 13 months ago by
Commit:  5beba14101f2a6c01d98766d4d1aca5caba5a4c5 → 3197cd345c8449d76632f8d5f820eb7949f026ff 

Branch pushed to git repo; I updated commit sha1. New commits:
3197cd3  Merge branch 'develop' into public/combinat/image_sets32121

comment:28 Changed 13 months ago by
Authors:  Matthias Koeppe → Matthias Koeppe, Travis Scrimshaw 

Reviewers:  Travis Scrimshaw → Travis Scrimshaw, ... 
comment:29 Changed 12 months ago by
Commit:  3197cd345c8449d76632f8d5f820eb7949f026ff → b845bfb437b1027b8ea7b41e8e39f139ec4dd578 

Branch pushed to git repo; I updated commit sha1. New commits:
b845bfb  Merge branch 'develop' into public/combinat/image_sets32121

comment:30 Changed 12 months ago by
Milestone:  sage9.5 → sage9.6 

comment:31 Changed 10 months ago by
Commit:  b845bfb437b1027b8ea7b41e8e39f139ec4dd578 → 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 10 months ago by
Commit:  7c091af52930bfcd80a94d177c61d5f9be8f0ae0 → 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 10 months ago by
Commit:  0d850f1c90df828c69d45570b4ada0b925115312 → 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 10 months ago by
Commit:  0b70dc93b61a5a5e7f40e388d392af80da62dbc4 → f8d559aafdf43b1e256210a76bd025751adff8bb 

comment:35 Changed 10 months ago by
Reviewers:  Travis Scrimshaw, ... → Travis Scrimshaw, Matthias Koeppe 

comment:36 Changed 10 months ago by
Commit:  f8d559aafdf43b1e256210a76bd025751adff8bb → 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 10 months ago by
Commit:  6605ad26fcfcd1fad253c0b4e18f12b2bed8ff18 → 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 10 months ago by
Commit:  4bff6c5b1c8cdec7947ffd653a324086bbfd38a2 → 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 10 months ago by
Commit:  7c6aacffb39bc16c000ee098a229ec692d9a176b → 772970d8693af5e5ef10682a6b9c31dee030fc4b 

Branch pushed to git repo; I updated commit sha1. New commits:
772970d  src/sage/categories/enumerated_sets.py: Remove outdated warning

comment:41 Changed 10 months ago by
Commit:  772970d8693af5e5ef10682a6b9c31dee030fc4b → 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 10 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 Changed 10 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 10 months ago by
Commit:  bbc2f02fdcbbc482bb175f3a672aa95f859cefbd → 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 10 months ago by
Commit:  d3a90ee9dcfe765a477641d190aa51bfede7ea23 → 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:47 Changed 10 months ago by
Status:  needs_review → positive_review 

Patchbot failures are unrelated.
comment:48 Changed 10 months ago by
Status:  positive_review → 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 10 months ago by
Commit:  0e9431ce03fe40f069f7afac52f9172052de439e → 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 10 months ago by
Status:  needs_work → positive_review 

comment:51 Changed 9 months ago by
Branch:  public/combinat/image_sets32121 → 038e354711832119926df56e411759d44f252d80 

Resolution:  → fixed 
Status:  positive_review → 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