words.CharacteristicSturmianWord does not do what it says

[seblabbe]

The doc of words.CharacteristicSturmianWord says :

INPUT:
-  ``cf`` - an iterator outputting integers (thought of as a
               continued fraction)

But it does not do what it says. In fact the following

sage: cf = CFF(1/golden_ratio^2)
sage: words.CharacteristicSturmianWord(cf)
word: 0010001001000100010010001001000100010010...

should output the same as

sage: words.FibonacciWord()
word: 0100101001001010010100100101001001010010...

CC: @sagetrac-abmasse

Component: combinatorics

Author: Sébastien Labbé

Reviewer: Alexandre Blondin Massé

Merged: sage-4.3.3.alpha0

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

[seblabbe]

Attachment: trac_8140-sturmian-sl.patch.gz

[seblabbe]

comment:2

I just uploaded the patch : I corrected a sphinx warning. I hope it will not create conflicts if Alexandre started a review...

[sagetrac-abmasse]

comment:3

I reviewed this patch and made the following minor modifications, mostly in the documentation:

• I gave three different equivalent definitions of characteristic Sturmian word as presented in the Lothaire book.
• I changed the name of the variable cf for slope in the characteristic Sturmian.
• I modified the NotImplementedError raised by the three functions by a ValueError. It seems more appropriate since values of slope are in general assumed to be in (0,1). Sébastien, if you still insist on keeping NotImplementedError, I would put a different message, something like "not implemented for values out of (0,1)"
• I made other minor changes and updated the examples in consequence.
  All tests passed, the code seems good and correct the problem mentionned in this ticket. The doc is alright too.

[sagetrac-abmasse]

Author: slabbe

[sagetrac-abmasse]

Reviewer: abmasse

[sagetrac-abmasse]

Attachment: trac_8140-doc_fixes-abm.patch.gz

Few minor changes -- I let Sébastien check if he approves the changes

[seblabbe]

Attachment: trac_8140_cf-arg-sl.patch.gz

Applies over the two precedent patches.

[seblabbe]

comment:4

I agree with your changes. I fix the doc (the irrationality of alpha is necessary for the lower and upper mechanical word to be equal). I also added rename_keyword of the cf argument that you removed for backward compatibility.

I give a positive review to Alexandre's changes. Alexandre, I let you change the status of the ticket to positive review if you agree with my two patches.

[seblabbe]

Changed author from slabbe to Sébastien Labbé

[seblabbe]

comment:5

Full name in those boxes helps the release managers when writing the release notes.

[seblabbe]

Changed reviewer from abmasse to Alexandre Blondin-Massé

[sagetrac-abmasse]

Changed reviewer from Alexandre Blondin-Massé to Alexandre Blondin Massé

[sagetrac-abmasse]

comment:6

Rechecked the three functions after applying all three patches and everything looks fine. All tests passed, the doc built with Sphinx looks alright too and I agree with the last minor changes of Sébastien. Positive review as well !
Thanks for the info about the full names in the boxes, I wasn't sure what to do about that.

[qed777]

comment:7

The commit string for the third patch is not sufficiently descriptive. I've refreshed it in my queue for 4.3.3.alpha0: #8140: Added rename_keyword for the cf argument. Please let me know if this is not good enough!

[seblabbe]

comment:8

Replying to @qed777:

The commit string for the third patch is not sufficiently descriptive. I've refreshed it in my queue for 4.3.3.alpha0: #8140: Added rename_keyword for the cf argument. Please let me know if this is not good enough!

It is perfect (sorry, I forgot to write the description).

[qed777]

Merged: sage-4.3.3.alpha0