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: sage-5.12
Component: group theory Keywords:
Cc: rbeezer Merged in: sage-5.12.beta0
Authors: Davis Shurbert Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by dshurbert)

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)

trac_14790_fpg_names.patch (2.4 KB) - added by dshurbert 8 years ago.
Replacement patch, requested changes made, added commit message

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by dshurbert

  • Description modified (diff)

comment:2 Changed 8 years ago by dshurbert

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by dshurbert

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

comment:4 Changed 8 years ago by rbeezer

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 rbeezer

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.
  1. And, flag as a keyword name is totally uninformative. Use something with some meaning. Help the reader. I'd suggest zeros=False.
  1. seed is not a seed. It is a counter. Call it count or i or something else.
  1. 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.
  1. 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.

comment:6 Changed 8 years ago by dshurbert

Made changes suggested above.

comment:7 Changed 8 years ago by rbeezer

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

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

comment:9 Changed 8 years ago by rbeezer

  • Status changed from positive_review to needs_info

comment:10 Changed 8 years ago by rbeezer

  • Status changed from needs_info to needs_review

comment:11 Changed 8 years ago by rbeezer

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

  • Milestone changed from sage-5.11 to sage-5.12

comment:13 Changed 8 years ago by jdemeyer

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

comment:14 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Changed 8 years ago by dshurbert

Replacement patch, requested changes made, added commit message

comment:15 Changed 8 years ago by dshurbert

  • Status changed from needs_info to needs_review

Just added commit message, ready for re-review.

comment:16 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:17 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.