Opened 7 years ago
Closed 4 years ago
#17039 closed enhancement (fixed)
Allow IndexedGenerators to handle `names`
Reported by:  tscrim  Owned by:  tscrim 

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  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 7 years ago by
 Dependencies set to #17035
comment:2 Changed 7 years ago by
 Branch set to public/misc/names_argument_indexed_generators17039
 Commit set to d23726c2fbfa5144ac3d0223907c54f28ed37bbe
 Status changed from new to needs_review
comment:3 Changed 7 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_generators17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators17039

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_generators17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators17039

comment:5 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.8
comment:6 Changed 6 years ago by
 Commit changed from 1aae120b34f5fb96d4ab261dd37c09ee5a402e51 to e3175db7d92ff2e03d7a2dba5baab4ab456c2f59
comment:7 Changed 6 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_generators17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators17039

ee0536e  Added (indexed) monoids to finitely generated monoids category.

63e54b0  Merge branch 'public/monoids/free_monoids_finitely_generated20405' into public/misc/names_argument_indexed_generators17039

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_semigroups20403' into public/misc/names_argument_indexed_generators17039

fc9a4f8  Being more python3 compatible.

comment:8 Changed 6 years ago by
 Dependencies changed from #17035 to #17035 #20403 #20405
 Milestone changed from sage6.8 to sage7.2
comment:9 Changed 6 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_generated20405' into public/misc/names_argument_indexed_generators17039

comment:10 Changed 6 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_semigroups20403' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators17039

e640249  Last fix of doctests.

2d48ce1  Merge branch 'public/monoids/free_monoids_finitely_generated20405' into public/misc/names_argument_indexed_generators17039

02df281  Fixing doctests.

comment:11 Changed 6 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_semigroups20403' of trac.sagemath.org:sage into public/monoids/free_monoids_finitely_generated20405

e7914d4  Pulling changes from #17039 to fix doctest with this (#20405) + #20403.

94f081b  Merge branch 'public/monoids/free_monoids_finitely_generated20405' into public/misc/names_argument_indexed_generators17039

comment:12 Changed 6 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_semigroups20403

fc1eb23  Merge branch 'public/misc/names_argument_indexed_generators17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators17039

comment:13 Changed 6 years ago by
 Commit changed from fc1eb23fb2d7fb16bdbb5011e7a1df3f437ef113 to 0dfc850e676eca3f6869c24f9e59264317f83465
comment:14 Changed 5 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 5 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_generators17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators17039

comment:16 Changed 5 years ago by
 Milestone changed from sage7.2 to sage8.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 followups: ↓ 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 ; followup: ↓ 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_generators17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators17039

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_generators17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators17039

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_generators17039 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.