Opened 6 years ago
Closed 4 years ago
#17039 closed enhancement (fixed)
Allow IndexedGenerators to handle `names`
Reported by: | tscrim | Owned by: | tscrim |
---|---|---|---|
Priority: | major | Milestone: | sage-8.0 |
Component: | misc | Keywords: | variable names input |
Cc: | nthiery, nbruin, chapoton, bsalisbury1 | Merged in: | |
Authors: | Travis Scrimshaw | Reviewers: | Jeroen Demeyer, Ben Salisbury |
Report Upstream: | N/A | Work issues: | |
Branch: | 73cf1a7 (Commits) | Commit: | 73cf1a7607f2b36e71eec361646c758e0cd725c9 |
Dependencies: | #17035 #20403 #20405 | Stopgaps: |
Description
So we can do things like
sage: C.<x,y,z> = CombinatorialFreeModule(QQ)
Change History (36)
comment:1 Changed 6 years ago by
- Dependencies set to #17035
comment:2 Changed 6 years ago by
- Branch set to public/misc/names_argument_indexed_generators-17039
- Commit set to d23726c2fbfa5144ac3d0223907c54f28ed37bbe
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Commit changed from d23726c2fbfa5144ac3d0223907c54f28ed37bbe to 06e7a3c1b18ec7ca5b6196ea5b6bc9a91371792c
Branch pushed to git repo; I updated commit sha1. New commits:
06e7a3c | Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
|
comment:4 Changed 6 years ago by
- Commit changed from 06e7a3c1b18ec7ca5b6196ea5b6bc9a91371792c to 1aae120b34f5fb96d4ab261dd37c09ee5a402e51
Branch pushed to git repo; I updated commit sha1. New commits:
1aae120 | Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
|
comment:5 Changed 6 years ago by
- Milestone changed from sage-6.4 to sage-6.8
comment:6 Changed 5 years ago by
- Commit changed from 1aae120b34f5fb96d4ab261dd37c09ee5a402e51 to e3175db7d92ff2e03d7a2dba5baab4ab456c2f59
comment:7 Changed 5 years ago by
- Commit changed from e3175db7d92ff2e03d7a2dba5baab4ab456c2f59 to fc9a4f867d726d0d3ba97f2e320d8711d45a84e7
Branch pushed to git repo; I updated commit sha1. New commits:
84792f6 | Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
|
ee0536e | Added (indexed) monoids to finitely generated monoids category.
|
63e54b0 | Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039
|
a0307f3 | Changed iterator for finitely generated semigroups.
|
c427ca9 | Change TODO list in backtrack to state we can deprecate TransitiveIdeal* now.
|
dfa5c75 | Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' into public/misc/names_argument_indexed_generators-17039
|
fc9a4f8 | Being more python3 compatible.
|
comment:8 Changed 5 years ago by
- Dependencies changed from #17035 to #17035 #20403 #20405
- Milestone changed from sage-6.8 to sage-7.2
comment:9 Changed 5 years ago by
- Commit changed from fc9a4f867d726d0d3ba97f2e320d8711d45a84e7 to 52f7f98feba694f3eb3034f282d8787867baa990
Branch pushed to git repo; I updated commit sha1. New commits:
0c3452d | Changing behavior of _first_ngens to support using gens().
|
c95e024 | Fixing trivial failing doctests due to new iterator.
|
52f7f98 | Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039
|
comment:10 Changed 5 years ago by
- Commit changed from 52f7f98feba694f3eb3034f282d8787867baa990 to 02df281bd3b0f283d0355ae4a4e203d3741c66e5
Branch pushed to git repo; I updated commit sha1. New commits:
a27f383 | trac #20403 fixing one doctest
|
c20562e | trac #20403 doc formatting
|
fabe683 | Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
|
e640249 | Last fix of doctests.
|
2d48ce1 | Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039
|
02df281 | Fixing doctests.
|
comment:11 Changed 5 years ago by
- Commit changed from 02df281bd3b0f283d0355ae4a4e203d3741c66e5 to 94f081bb4694777c9c2cce1fe9bd506bdba6a679
Branch pushed to git repo; I updated commit sha1. New commits:
1c07d56 | Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' of trac.sagemath.org:sage into public/monoids/free_monoids_finitely_generated-20405
|
e7914d4 | Pulling changes from #17039 to fix doctest with this (#20405) + #20403.
|
94f081b | Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039
|
comment:12 Changed 5 years ago by
- Commit changed from 94f081bb4694777c9c2cce1fe9bd506bdba6a679 to fc1eb23fb2d7fb16bdbb5011e7a1df3f437ef113
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
569de0d | Merge branch 'develop' into t/13580/map_reduce
|
b07dfe2 | Doc of the two implementationsof ActiveTaskCounter
|
beebcbc | #13580: Added comment on timing in the doc
|
58eca2e | #13580: Removed comment which is now in the doc
|
1badd8a | #13580: Renamed ActiveTaskCounter(Posix)
|
46cbab9 | 13580: Fixed doctests to pass on Darwin
|
134c1fa | 13580: doc rereading
|
4c82d1b | Merge branch 'u/hivert/ticket/13580' of trac.sagemath.org:sage into u/hivert/ticket/13580
|
67521dd | Merge branch 'u/hivert/ticket/13580' into public/semigroups/iterator_finitely_generated_semigroups-20403
|
fc1eb23 | Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
|
comment:13 Changed 5 years ago by
- Commit changed from fc1eb23fb2d7fb16bdbb5011e7a1df3f437ef113 to 0dfc850e676eca3f6869c24f9e59264317f83465
comment:14 Changed 4 years ago by
- Status changed from needs_review to needs_work
I don't care much about this ticket, just pointing out that it no longer applies.
comment:15 Changed 4 years ago by
- Commit changed from 0dfc850e676eca3f6869c24f9e59264317f83465 to 5f5bb7612d84dad01fe36b0fb6d019c0662886d1
Branch pushed to git repo; I updated commit sha1. New commits:
5f5bb76 | Merge branch 'public/misc/names_argument_indexed_generators-17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators-17039
|
comment:16 Changed 4 years ago by
- Milestone changed from sage-7.2 to sage-8.0
- Status changed from needs_work to needs_review
comment:17 Changed 4 years ago by
Green patchbot (essentially).
comment:18 Changed 4 years ago by
This is not good:
@@ -127,6 +131,8 @@ def FreeMonoid(index_set=None, names=None, commutative=False, **kwds): if index_set not in ZZ: if names is not None: + if isinstance(names, str): + names = names.split(',') names = normalize_names(len(names), names) from sage.monoids.indexed_free_monoid import IndexedFreeMonoid return IndexedFreeMonoid(index_set, names=names, **kwds)
This splitting should be handled by normalize_names()
(I guess you should replace len(names)
by -1
).
I also wonder why you do not use normalize_names()
in parse_indices_names()
. The function normalize_names()
is precisely meant to handle names
arguments consistently. Don't reinvent the wheel!
comment:19 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:20 Changed 4 years ago by
comment:21 Changed 4 years ago by
- Commit changed from 5f5bb7612d84dad01fe36b0fb6d019c0662886d1 to 627e5e081727b9abf1466184c71b37c4b2c081ca
comment:22 Changed 4 years ago by
- Commit changed from 627e5e081727b9abf1466184c71b37c4b2c081ca to 06eae3366537041b90e8fe22d6549dd0b7b8ed36
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
06eae33 | Removing code duplication.
|
comment:23 follow-ups: ↓ 25 ↓ 26 Changed 4 years ago by
- Status changed from needs_work to needs_review
Thank you for looking at this.
comment:18 Good point. IIRC, back when I did this, that was not separated as a standalone function but instead a method of CategoryObject
. I also made some other changes to remove code duplication and improve the documentation.
comment:20 I don't want to do that because other keyword options that are used should not default to False
, such as scalar_mult
.
comment:24 Changed 4 years ago by
- Commit changed from 06eae3366537041b90e8fe22d6549dd0b7b8ed36 to 76b3be2dd4123baaff90dc38c23d11d3e568df1c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
76b3be2 | Removing code duplication.
|
comment:25 in reply to: ↑ 23 ; follow-up: ↓ 28 Changed 4 years ago by
Replying to tscrim:
comment:20 I don't want to do that because other keyword options that are used should not default to
False
, such asscalar_mult
.
I don't get your point. I am saying that you can replace
if 'bracket' not in kwds: kwds['bracket'] = False
by
kwds.setdefault('bracket', False)
You can do the same for string_quotes
but I'm not saying that you must do this for every keyword if you don't want.
comment:26 in reply to: ↑ 23 Changed 4 years ago by
Replying to tscrim:
comment:18 Good point. IIRC, back when I did this, that was not separated as a standalone function but instead a method of
CategoryObject
.
Yes, this was fixed in #19675.
comment:27 Changed 4 years ago by
- Commit changed from 76b3be2dd4123baaff90dc38c23d11d3e568df1c to 8f7d3e635d568228ea76c130feb486ec1317c1f9
Branch pushed to git repo; I updated commit sha1. New commits:
8f7d3e6 | Use setdefault instead of manually checking.
|
comment:28 in reply to: ↑ 25 Changed 4 years ago by
Replying to jdemeyer:
Replying to tscrim:
comment:20 I don't want to do that because other keyword options that are used should not default to
False
, such asscalar_mult
.I don't get your point. I am saying that you can replace
if 'bracket' not in kwds: kwds['bracket'] = Falseby
kwds.setdefault('bracket', False)You can do the same for
string_quotes
but I'm not saying that you must do this for every keyword if you don't want.
I misread setdefault
. I was thinking it made the dict
behave like a defaultdict
. I agree that this is a better way to go.
comment:29 Changed 4 years ago by
Patchbot is back to (essentially) green.
comment:30 Changed 4 years ago by
- Commit changed from 8f7d3e635d568228ea76c130feb486ec1317c1f9 to 06f1536a17c16c6855fa960f2dacb377b95d4274
Branch pushed to git repo; I updated commit sha1. New commits:
06f1536 | Merge branch 'public/misc/names_argument_indexed_generators-17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators-17039
|
comment:31 Changed 4 years ago by
- Commit changed from 06f1536a17c16c6855fa960f2dacb377b95d4274 to 451d60b9d6bd32e79a6bbcfe05b19ee0e20b37b9
Branch pushed to git repo; I updated commit sha1. New commits:
451d60b | Merge branch 'public/misc/names_argument_indexed_generators-17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators-17039
|
comment:32 Changed 4 years ago by
- Commit changed from 451d60b9d6bd32e79a6bbcfe05b19ee0e20b37b9 to 73cf1a7607f2b36e71eec361646c758e0cd725c9
Branch pushed to git repo; I updated commit sha1. New commits:
73cf1a7 | Updating split_index_keywords.
|
comment:34 Changed 4 years ago by
- Reviewers set to Ben Salisbury
- Status changed from needs_review to positive_review
Changes look good and everything builds and passes on my machine.
comment:35 Changed 4 years ago by
- Reviewers changed from Ben Salisbury to Jeroen Demeyer, Ben Salisbury
Thank you.
comment:36 Changed 4 years ago by
- Branch changed from public/misc/names_argument_indexed_generators-17039 to 73cf1a7607f2b36e71eec361646c758e0cd725c9
- Resolution set to fixed
- Status changed from positive_review to closed
Done and up for review.
New commits:
Another optio for indexed generators.
Fixed failing doctest.
Added support for C.<x,...> for CombinatorialFreeModule.