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: |
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 (Meta-ticket: 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 gh-mwageringel 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 --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
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 sage-9.4 to sage-9.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_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:
8d87ea5 | Merge branch 'u/mkoeppe/replace_mapcombinatorialclass__add_methods_map_image__map_pushforward' of git://trac.sagemath.org/sage into public/combinat/image_sets-32121
|
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_sets-32121
|
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_sets-32121
|
comment:30 Changed 5 months ago by
- Milestone changed from sage-9.5 to sage-9.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_sets-32121
|
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 follow-up: ↓ 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 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
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 one-line description
|
b301b59 | src/sage/sets/image_set.py: Fix one-line 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 --random-seed=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_sets-32121 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