Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python generator for free group variable names #14790

Closed
sagetrac-dshurbert mannequin opened this issue Jun 21, 2013 · 13 comments
Closed

Python generator for free group variable names #14790

sagetrac-dshurbert mannequin opened this issue Jun 21, 2013 · 13 comments

Comments

@sagetrac-dshurbert
Copy link
Mannequin

sagetrac-dshurbert mannequin commented Jun 21, 2013

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

CC: @rbeezer

Component: group theory

Author: Davis Shurbert

Reviewer: Rob Beezer

Merged: sage-5.12.beta0

Issue created by migration from https://trac.sagemath.org/ticket/14790

@sagetrac-dshurbert sagetrac-dshurbert mannequin added this to the sage-5.11 milestone Jun 21, 2013
@sagetrac-dshurbert

This comment has been minimized.

@sagetrac-dshurbert
Copy link
Mannequin Author

sagetrac-dshurbert mannequin commented Jun 21, 2013

comment:3

Added option to specify whether or not initial iteration through alphabet appends '0' to
output string

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Jun 21, 2013

comment:4

To test this function, first run

from sage.groups.free_group import _lexi_gen

then

lex = _lexi_gen()

then repeated use of

lex.next()

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Jul 2, 2013

comment:5
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.
  1. And, flag as a keyword name is totally uninformative. Use something with some meaning. Help the reader. I'd suggest zeros=False.

  2. seed is not a seed. It is a counter. Call it count or i or something else.

  3. 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 toss-up.

  1. while (True): does not need parentheses.

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

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

  1. Changing flag inside your if/else on flag is very confusing. How about
if mwrap == 0 and not(zeros):
    name=''
else:
    name = str(mwrap)

and you can drop the line initializing name.

@sagetrac-dshurbert
Copy link
Mannequin Author

sagetrac-dshurbert mannequin commented Jul 11, 2013

comment:6

Made changes suggested above.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Jul 12, 2013

comment:7

Looks good! Passes tests on 5.11.beta3. Positive review.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Jul 12, 2013

Reviewer: Rob Beezer

@sagetrac-dshurbert
Copy link
Mannequin Author

sagetrac-dshurbert mannequin commented Jul 14, 2013

comment:8

Just moved count increment to top of code for ease of reader.

@rbeezer rbeezer mannequin added s: needs info and removed s: positive review labels Jul 14, 2013
@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Jul 14, 2013

comment:11

Change looks good, and passes all necessary testing. So back to "positive review".

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Jul 21, 2013
@jdemeyer
Copy link

comment:13

The patch needs a proper commit message (use hg qrefresh -e for that).

@sagetrac-dshurbert
Copy link
Mannequin Author

sagetrac-dshurbert mannequin commented Jul 31, 2013

Replacement patch, requested changes made, added commit message

@sagetrac-dshurbert
Copy link
Mannequin Author

sagetrac-dshurbert mannequin commented Jul 31, 2013

comment:15

Attachment: trac_14790_fpg_names.patch.gz

Just added commit message, ready for re-review.

@jdemeyer
Copy link

jdemeyer commented Aug 2, 2013

Merged: sage-5.12.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants