Opened 8 years ago

Closed 8 years ago

## #14790 closed enhancement (fixed)

Reported by: Owned by: dshurbert joyner minor sage-5.12 group theory rbeezer sage-5.12.beta0 Davis Shurbert Rob Beezer N/A

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

### 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: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

### comment:15 Changed 8 years ago by dshurbert

• Status changed from needs_info to needs_review