Opened 8 years ago
Closed 8 years ago
#14790 closed enhancement (fixed)
Python generator for free group variable names
Reported by:  dshurbert  Owned by:  joyner 

Priority:  minor  Milestone:  sage5.12 
Component:  group theory  Keywords:  
Cc:  rbeezer  Merged in:  sage5.12.beta0 
Authors:  Davis Shurbert  Reviewers:  Rob Beezer 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
When creating free and finitely presented groups, default variable names are often ugly.
sage: F = FreeGroup(12) sage: F Free Group on generators {x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11} sage: F/[F([1,2,2,1,1,4,7,8]),F([10,10,4,5,4,8]) ] Finitely presented group < x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11  x0*x1^2*x0^2*x3*x6*x7^1, x9^2*x3*x4^1*x3*x7^1 >
Created by a utility function, the simple generator in this patch provides an easy way to consistently generate better variable names, and will be useful in creating free groups algorithmically.
Apply
Attachments (1)
Change History (18)
comment:1 Changed 8 years ago by
 Description modified (diff)
comment:2 Changed 8 years ago by
 Status changed from new to needs_review
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
To test this function, first run
from sage.groups.free_group import _lexi_gen
then
lex = _lexi_gen()
then repeated use of
lex.next()
comment:5 Changed 8 years ago by
1.
Utility method to create generator for free group variable names, for use when creating free groups of dynamic size.
"generator" has two meanings here (and I confused the two). Better:
Return a generator object that produces names suitable for the generators of a free group.
 And,
flag
as a keyword name is totally uninformative. Use something with some meaning. Help the reader. I'd suggestzeros=False
.

seed
is not a seed. It is a counter. Call itcount
ori
or something else.
 Do you really need to import "string" and use the
lowercase
array? The usual idiom is:chr(ord('a') + ind)
I guess this one is a tossup.

while (True):
does not need parentheses.
 Include an EXAMPLE doctest that shows a higher iteration through the alphabet, maybe show
ls = [test.next() for i in range(3*26)] ls[2*26:3*26]
You can break the long list of output in the middle at whitespace and the test will succeed.
 The doctest in the TESTS section is great as is, though you could put both outputs on one line:
ls[234], ls[260]
and get a pair as the output. Just more concise.
 Changing
flag
inside your if/else onflag
is very confusing. How aboutif mwrap == 0 and not(zeros): name='' else: name = str(mwrap)
and you can drop the line initializing name
.
comment:6 Changed 8 years ago by
Made changes suggested above.
comment:7 Changed 8 years ago by
 Reviewers set to Rob Beezer
 Status changed from needs_review to positive_review
Looks good! Passes tests on 5.11.beta3. Positive review.
comment:8 Changed 8 years ago by
Just moved count
increment to top of code for ease of reader.
comment:9 Changed 8 years ago by
 Status changed from positive_review to needs_info
comment:10 Changed 8 years ago by
 Status changed from needs_info to needs_review
comment:11 Changed 8 years ago by
 Status changed from needs_review to positive_review
Change looks good, and passes all necessary testing. So back to "positive review".
comment:12 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:13 Changed 8 years ago by
The patch needs a proper commit message (use hg qrefresh e
for that).
comment:14 Changed 8 years ago by
 Status changed from positive_review to needs_info
comment:15 Changed 8 years ago by
 Status changed from needs_info to needs_review
Just added commit message, ready for rereview.
comment:16 Changed 8 years ago by
 Status changed from needs_review to positive_review
comment:17 Changed 8 years ago by
 Merged in set to sage5.12.beta0
 Resolution set to fixed
 Status changed from positive_review to closed
Added option to specify whether or not initial iteration through alphabet appends '0' to output string