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: sage-9.6
Component: combinatorics Keywords:
Cc: tscrim, chapoton, roed, gh-mwageringel 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 mkoeppe)

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 11 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 11 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 11 months ago by mkoeppe

  • Cc roed added
  • Summary changed from Replace MapCombinatorialClass to Replace MapCombinatorialClass, add methods Map.image, Map.pushforward

comment:4 Changed 11 months ago by mkoeppe

  • Branch set to u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward

comment:5 Changed 11 months ago by git

  • Commit set to 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 11 months ago by mkoeppe

  • Dependencies set to #32013

comment:7 Changed 11 months ago by git

  • Commit changed from fef2e030ff2a276ae2dea39e26c628a598806c5e to 8e920e9a358241bd330ffaaf832a73c1a24c363a

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 11 months ago by git

  • Commit changed from 8e920e9a358241bd330ffaaf832a73c1a24c363a to b57c6a5c19860c18884f4b7e5eba25353cad95d3

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

b57c6a5ImageSubobject: Handle is_injective, fix repr

comment:9 Changed 11 months ago by git

  • Commit changed from b57c6a5c19860c18884f4b7e5eba25353cad95d3 to 5223859004d476310b1c24f06752e1f03ca363be

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

5223859ImageSubobject: Fixup finite/infinite

comment:10 Changed 11 months ago by mkoeppe

  • Authors set to Matthias Koeppe

Here's an attempt

comment:11 Changed 11 months ago by git

  • Commit changed from 5223859004d476310b1c24f06752e1f03ca363be to 2d0bd77aec3509491c20d1b93129f76d3f73ba97

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

2d0bd77Map.image: Fixup

comment:12 Changed 11 months ago by mkoeppe

  • Cc gh-mwageringel added

comment:13 Changed 11 months ago by git

  • Commit changed from 2d0bd77aec3509491c20d1b93129f76d3f73ba97 to 7d362129096d2dcdb675e057be69aed83f2fc38a

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

7d36212ImageSubobject: Handle CallableSymbolicExpression as map

comment:14 Changed 11 months ago by mkoeppe

  • 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 mkoeppe

  • Dependencies changed from #32013 to #32013, #32130

comment:16 Changed 11 months ago by git

  • Commit changed from 7d362129096d2dcdb675e057be69aed83f2fc38a to f80d5f4607a158911ff0f8e1d047161a5c12d0f2

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 11 months ago by mkoeppe

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

comment:18 Changed 10 months ago by mkoeppe

  • Status changed from new to needs_review

comment:19 Changed 10 months ago by git

  • Commit changed from f80d5f4607a158911ff0f8e1d047161a5c12d0f2 to 84eec2a920ef556b21fe496a734afe6375193564

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 10 months ago by tscrim

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 10 months ago by git

  • Commit changed from 84eec2a920ef556b21fe496a734afe6375193564 to 9e97ea0b9d328a67ffd4e7cb47780a4b6421d6e7

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 10 months ago by mkoeppe

  • Dependencies #32013, #32130 deleted

Rebased, partially squashed, on 9.4.rc0

comment:23 Changed 10 months ago by tscrim

  • 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 --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 10 months ago by mkoeppe

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 mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:26 Changed 8 months ago by tscrim

  • Branch changed from u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward to public/combinat/image_sets-32121
  • 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:

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 6 months ago by git

  • Commit changed from 5beba14101f2a6c01d98766d4d1aca5caba5a4c5 to 3197cd345c8449d76632f8d5f820eb7949f026ff

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

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

comment:28 Changed 6 months ago by mkoeppe

  • Authors changed from Matthias Koeppe to Matthias Koeppe, Travis Scrimshaw
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, ...

comment:29 Changed 5 months ago by git

  • Commit changed from 3197cd345c8449d76632f8d5f820eb7949f026ff to b845bfb437b1027b8ea7b41e8e39f139ec4dd578

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

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

comment:30 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:31 Changed 3 months ago by git

  • Commit changed from b845bfb437b1027b8ea7b41e8e39f139ec4dd578 to 7c091af52930bfcd80a94d177c61d5f9be8f0ae0

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 3 months ago by git

  • Commit changed from 7c091af52930bfcd80a94d177c61d5f9be8f0ae0 to 0d850f1c90df828c69d45570b4ada0b925115312

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 3 months ago by git

  • Commit changed from 0d850f1c90df828c69d45570b4ada0b925115312 to 0b70dc93b61a5a5e7f40e388d392af80da62dbc4

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

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

comment:34 Changed 3 months ago by git

  • Commit changed from 0b70dc93b61a5a5e7f40e388d392af80da62dbc4 to f8d559aafdf43b1e256210a76bd025751adff8bb

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

8688280WIP
f8d559aPoorManMap._sympy_: Add doctest

comment:35 Changed 3 months ago by mkoeppe

  • Reviewers changed from Travis Scrimshaw, ... to Travis Scrimshaw, Matthias Koeppe

comment:36 Changed 3 months ago by git

  • Commit changed from f8d559aafdf43b1e256210a76bd025751adff8bb to 6605ad26fcfcd1fad253c0b4e18f12b2bed8ff18

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

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

comment:37 Changed 3 months ago by git

  • Commit changed from 6605ad26fcfcd1fad253c0b4e18f12b2bed8ff18 to 4bff6c5b1c8cdec7947ffd653a324086bbfd38a2

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 3 months ago by git

  • Commit changed from 4bff6c5b1c8cdec7947ffd653a324086bbfd38a2 to 7c6aacffb39bc16c000ee098a229ec692d9a176b

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

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

comment:39 Changed 3 months ago by git

  • Commit changed from 7c6aacffb39bc16c000ee098a229ec692d9a176b to 772970d8693af5e5ef10682a6b9c31dee030fc4b

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

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

comment:40 Changed 3 months ago by mkoeppe

I think this is ready now

comment:41 Changed 3 months ago by git

  • Commit changed from 772970d8693af5e5ef10682a6b9c31dee030fc4b to bbc2f02fdcbbc482bb175f3a672aa95f859cefbd

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

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

comment:42 follow-up: Changed 3 months ago by tscrim

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 3 months ago by mkoeppe

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 git

  • Commit changed from bbc2f02fdcbbc482bb175f3a672aa95f859cefbd to d3a90ee9dcfe765a477641d190aa51bfede7ea23

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 3 months ago by git

  • Commit changed from d3a90ee9dcfe765a477641d190aa51bfede7ea23 to 0e9431ce03fe40f069f7afac52f9172052de439e

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 3 months ago by tscrim

Thank you. Green bot => positive review.

comment:47 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Patchbot failures are unrelated.

comment:48 Changed 3 months ago by vbraun

  • 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 --random-seed=0 src/sage/sets/image_set.py  # 1 doctest failed
----------------------------------------------------------------------

comment:49 Changed 3 months ago by git

  • Commit changed from 0e9431ce03fe40f069f7afac52f9172052de439e to 038e354711832119926df56e411759d44f252d80

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 3 months ago by mkoeppe

  • Status changed from needs_work to positive_review

comment:51 Changed 3 months ago by vbraun

  • Branch changed from public/combinat/image_sets-32121 to 038e354711832119926df56e411759d44f252d80
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.