Opened 7 years ago

Closed 7 years ago

#8140 closed defect (fixed)

words.CharacteristicSturmianWord does not do what it says

Reported by: slabbe Owned by: sage-combinat
Priority: major Milestone: sage-4.3.3
Component: combinatorics Keywords:
Cc: abmasse Merged in: sage-4.3.3.alpha0
Authors: Sébastien Labbé Reviewers: Alexandre Blondin Massé
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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

Attachments (3)

trac_8140-sturmian-sl.patch (15.6 KB) - added by slabbe 7 years ago.
trac_8140-doc_fixes-abm.patch (7.7 KB) - added by abmasse 7 years ago.
Few minor changes -- I let Sébastien check if he approves the changes
trac_8140_cf-arg-sl.patch (2.0 KB) - added by slabbe 7 years ago.
Applies over the two precedent patches.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by slabbe

  • Cc abmasse added
  • Status changed from new to needs_review

Changed 7 years ago by slabbe

comment:2 Changed 7 years ago by slabbe

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

comment:3 Changed 7 years ago by abmasse

  • Authors set to slabbe
  • Reviewers set to abmasse

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.

Changed 7 years ago by abmasse

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

Changed 7 years ago by slabbe

Applies over the two precedent patches.

comment:4 Changed 7 years ago by slabbe

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.

comment:5 Changed 7 years ago by slabbe

  • Authors changed from slabbe to Sébastien Labbé
  • Reviewers changed from abmasse to Alexandre Blondin-Massé

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

comment:6 Changed 7 years ago by abmasse

  • Reviewers changed from Alexandre Blondin-Massé to Alexandre Blondin Massé
  • Status changed from needs_review to positive_review

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.

comment:7 follow-up: Changed 7 years ago by mpatel

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!

comment:8 in reply to: ↑ 7 Changed 7 years ago by slabbe

Replying to mpatel:

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

comment:9 Changed 7 years ago by mpatel

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