Opened 12 years ago
Closed 12 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)
Change History (12)
comment:1 Changed 12 years ago by
- Cc abmasse added
- Status changed from new to needs_review
Changed 12 years ago by
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
- 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
forslope
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.
comment:4 Changed 12 years ago by
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 12 years ago by
- 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 12 years ago by
- 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: ↓ 8 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
- Merged in set to sage-4.3.3.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
I just uploaded the patch : I corrected a sphinx warning. I hope it will not create conflicts if Alexandre started a review...