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: 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, GitHub, GitLab) Commit: 73cf1a7607f2b36e71eec361646c758e0cd725c9
Dependencies: #17035 #20403 #20405 Stopgaps:

Status badges

Description

So we can do things like

sage: C.<x,y,z> = CombinatorialFreeModule(QQ)

Change History (36)

comment:1 Changed 7 years ago by tscrim

  • Dependencies set to #17035

comment:2 Changed 7 years ago by tscrim

  • Branch set to public/misc/names_argument_indexed_generators-17039
  • Commit set to d23726c2fbfa5144ac3d0223907c54f28ed37bbe
  • Status changed from new to needs_review

Done and up for review.


New commits:

f94c3f8Another optio for indexed generators.
5b546c2Fixed failing doctest.
d23726cAdded support for C.<x,...> for CombinatorialFreeModule.

comment:3 Changed 7 years ago by git

  • Commit changed from d23726c2fbfa5144ac3d0223907c54f28ed37bbe to 06e7a3c1b18ec7ca5b6196ea5b6bc9a91371792c

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

06e7a3cMerge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039

comment:4 Changed 7 years ago by git

  • Commit changed from 06e7a3c1b18ec7ca5b6196ea5b6bc9a91371792c to 1aae120b34f5fb96d4ab261dd37c09ee5a402e51

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

1aae120Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039

comment:5 Changed 7 years ago by tscrim

  • Milestone changed from sage-6.4 to sage-6.8

comment:6 Changed 6 years ago by git

  • Commit changed from 1aae120b34f5fb96d4ab261dd37c09ee5a402e51 to e3175db7d92ff2e03d7a2dba5baab4ab456c2f59

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

7a387faMerge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
e3175dbFixing doctest ordering.

comment:7 Changed 6 years ago by git

  • Commit changed from e3175db7d92ff2e03d7a2dba5baab4ab456c2f59 to fc9a4f867d726d0d3ba97f2e320d8711d45a84e7

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

84792f6Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
ee0536eAdded (indexed) monoids to finitely generated monoids category.
63e54b0Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039
a0307f3Changed iterator for finitely generated semigroups.
c427ca9Change TODO list in backtrack to state we can deprecate TransitiveIdeal* now.
dfa5c75Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' into public/misc/names_argument_indexed_generators-17039
fc9a4f8Being more python3 compatible.

comment:8 Changed 6 years ago by tscrim

  • Dependencies changed from #17035 to #17035 #20403 #20405
  • Milestone changed from sage-6.8 to sage-7.2

comment:9 Changed 6 years ago by git

  • Commit changed from fc9a4f867d726d0d3ba97f2e320d8711d45a84e7 to 52f7f98feba694f3eb3034f282d8787867baa990

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

0c3452dChanging behavior of _first_ngens to support using gens().
c95e024Fixing trivial failing doctests due to new iterator.
52f7f98Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039

comment:10 Changed 6 years ago by git

  • Commit changed from 52f7f98feba694f3eb3034f282d8787867baa990 to 02df281bd3b0f283d0355ae4a4e203d3741c66e5

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

a27f383trac #20403 fixing one doctest
c20562etrac #20403 doc formatting
fabe683Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039
e640249Last fix of doctests.
2d48ce1Merge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039
02df281Fixing doctests.

comment:11 Changed 6 years ago by git

  • Commit changed from 02df281bd3b0f283d0355ae4a4e203d3741c66e5 to 94f081bb4694777c9c2cce1fe9bd506bdba6a679

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

1c07d56Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' of trac.sagemath.org:sage into public/monoids/free_monoids_finitely_generated-20405
e7914d4Pulling changes from #17039 to fix doctest with this (#20405) + #20403.
94f081bMerge branch 'public/monoids/free_monoids_finitely_generated-20405' into public/misc/names_argument_indexed_generators-17039

comment:12 Changed 6 years ago by git

  • Commit changed from 94f081bb4694777c9c2cce1fe9bd506bdba6a679 to fc1eb23fb2d7fb16bdbb5011e7a1df3f437ef113

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

569de0dMerge branch 'develop' into t/13580/map_reduce
b07dfe2Doc 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)
46cbab913580: Fixed doctests to pass on Darwin
134c1fa13580: doc rereading
4c82d1bMerge branch 'u/hivert/ticket/13580' of trac.sagemath.org:sage into u/hivert/ticket/13580
67521ddMerge branch 'u/hivert/ticket/13580' into public/semigroups/iterator_finitely_generated_semigroups-20403
fc1eb23Merge branch 'public/misc/names_argument_indexed_generators-17039' of trac.sagemath.org:sage into public/misc/names_argument_indexed_generators-17039

comment:13 Changed 6 years ago by git

  • Commit changed from fc1eb23fb2d7fb16bdbb5011e7a1df3f437ef113 to 0dfc850e676eca3f6869c24f9e59264317f83465

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

6272efdRemoving tab character.
0dfc850Merge branch 'public/semigroups/iterator_finitely_generated_semigroups-20403' into public/misc/names_argument_indexed_generators-17039

comment:14 Changed 5 years ago by jdemeyer

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

  • Commit changed from 0dfc850e676eca3f6869c24f9e59264317f83465 to 5f5bb7612d84dad01fe36b0fb6d019c0662886d1

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

5f5bb76Merge 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 5 years ago by tscrim

  • Milestone changed from sage-7.2 to sage-8.0
  • Status changed from needs_work to needs_review

comment:17 Changed 5 years ago by tscrim

Green patchbot (essentially).

comment:18 Changed 5 years ago by jdemeyer

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 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!

Version 1, edited 5 years ago by jdemeyer (previous) (next) (diff)

comment:19 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:20 Changed 5 years ago by jdemeyer

Instead of

if 'bracket' not in kwds:
    kwds['bracket'] = False

use setdefault

comment:21 Changed 5 years ago by git

  • Commit changed from 5f5bb7612d84dad01fe36b0fb6d019c0662886d1 to 627e5e081727b9abf1466184c71b37c4b2c081ca

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

ad23fbdMerge branch 'public/misc/names_argument_indexed_generators-17039' of git://trac.sagemath.org/sage into public/misc/names_argument_indexed_generators-17039
1c1cad1Use normalize_names.
627e5e0Removing code duplication.

comment:22 Changed 5 years ago by git

  • Commit changed from 627e5e081727b9abf1466184c71b37c4b2c081ca to 06eae3366537041b90e8fe22d6549dd0b7b8ed36

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

06eae33Removing code duplication.

comment:23 follow-ups: Changed 5 years ago by tscrim

  • 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 5 years ago by git

  • Commit changed from 06eae3366537041b90e8fe22d6549dd0b7b8ed36 to 76b3be2dd4123baaff90dc38c23d11d3e568df1c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

76b3be2Removing code duplication.

comment:25 in reply to: ↑ 23 ; follow-up: Changed 5 years ago by 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 as scalar_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 5 years ago by jdemeyer

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 5 years ago by git

  • Commit changed from 76b3be2dd4123baaff90dc38c23d11d3e568df1c to 8f7d3e635d568228ea76c130feb486ec1317c1f9

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

8f7d3e6Use setdefault instead of manually checking.

comment:28 in reply to: ↑ 25 Changed 5 years ago by tscrim

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 as scalar_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.

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 tscrim

Patchbot is back to (essentially) green.

comment:30 Changed 4 years ago by git

  • Commit changed from 8f7d3e635d568228ea76c130feb486ec1317c1f9 to 06f1536a17c16c6855fa960f2dacb377b95d4274

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

06f1536Merge 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 git

  • Commit changed from 06f1536a17c16c6855fa960f2dacb377b95d4274 to 451d60b9d6bd32e79a6bbcfe05b19ee0e20b37b9

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

451d60bMerge 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 git

  • Commit changed from 451d60b9d6bd32e79a6bbcfe05b19ee0e20b37b9 to 73cf1a7607f2b36e71eec361646c758e0cd725c9

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

73cf1a7Updating split_index_keywords.

comment:33 Changed 4 years ago by tscrim

  • Cc chapoton bsalisbury1 added

Rebased over beta12.

comment:34 Changed 4 years ago by bsalisbury1

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

  • Reviewers changed from Ben Salisbury to Jeroen Demeyer, Ben Salisbury

Thank you.

comment:36 Changed 4 years ago by vbraun

  • Branch changed from public/misc/names_argument_indexed_generators-17039 to 73cf1a7607f2b36e71eec361646c758e0cd725c9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.