New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add is_sturmian_factor, is_tangent methods for finite words #9877
Comments
This comment has been minimized.
This comment has been minimized.
comment:2
I'm testing your patch in the next two hours. Before testing it explicitly, just some comments:
I'm waiting for sage-combinat to install and I'll test and look more thoroughly at your code. |
comment:3
There is another problem with your function:
I think this shouldn't be the case. In fact, this is interesting, because it raises another problem I had when working on another patch. What I suggest is that you could first verify if the parent has size 2 and, if it's not the case, you check if its real alphabet has size 2 by using the expression I think the clean solution would be to replace the deprecated function
The first one is called by What I suggest is that you simply add the |
comment:4
I might be wrong, but it seems that the code for the |
comment:5
Replying to @sagetrac-abmasse:
Sure. This is a well known result (though there are many variants of this idea, i do not know whether this exact one is written somewhere), but i will add at least in the Arnoux chapter of the Pytheas Fogg book. I guess i will also add a reference to a recent article of Smillie and Ulcigrai which explains this in a nice way. |
comment:6
Replying to @sagetrac-abmasse:
ok. |
comment:7
Replying to @sagetrac-abmasse:
Actually, those words are the balanced one, but the method I do not really like Another possibility could be to add a parameter |
comment:8
Replying to @sagetrac-tmonteil:
This is a very good idea. I think you should do it. |
comment:9
Replying to @sagetrac-abmasse:
Since it is explained in the documentation, this is not a bug but a feature ;) I was thinking about this alphabet problem, and i am willing to write a ticket about that issue. I think that such a decision should be made for the whole word directory, with its whole community. Here are some bits. If i start to do such tests in such a method, then almost every method of the FiniteWords class will have to do it as well, and the faster parent().alphabet() method will become useless. So there is a problem of code replication. Another problem is that a test like Some programmer (user) should know where his/her words are living and should be warned if not. Making "friendly code" does not mean to pass over dirty programmer's inconsistencies, because then it won't detect the clean programmer's mistakes anymore. Fore those lazy ways of programming, we can imagine a global option which tells how the methods should care about the alphabet, so that both kinds of behavior are possible. As for me, this may look Bourbachist (in particular not If in some very few occasion, the user will indeed need to use such a method on some words defined on some non-two-letter alphabet, but then there should be some general coerce methods like Another example of why the alphabet question deals with the whole
|
comment:10
I think this is a very good suggestion. Sébastien |
comment:11
Hi, Thierry ! Sorry if I haven't been available lately (busy, you know, the usual justification...). If I'm not mistaken, there were last minor issues with your ticket that haven't been completely solved. To recap:
This is pretty much it! |
comment:12
We were on strike my friend. |
Tested on Sage 4.6 |
comment:13
Attachment: trac_9877_words_sturmian_desubstitution_attempt_2-tm.patch.gz Hi Alexandre, i started to work back on this ticket. This new patch is supposed to solve your four points. Since the Also, concerning a previous comment on using existing sage code, i looked for some "run" method and the only similar code i found was the
The first takes 28.6 ms per loop whereas the second takes 18.2 ms per loop. The problem is that the first method, though more low-level, is like passing twice on the word, which seems to be slower than the second (high-level) one-pass method. Ciao, |
Author: Thierry Monteil |
This comment has been minimized.
This comment has been minimized.
comment:14
Please fix the 5 test errors. Four of them are the same : AttributeError: 'WordGenerator' object has no attribute 'KolakoskiWord' See the report of the patchbot. |
This comment has been minimized.
This comment has been minimized.
comment:16
Salut Thierry,
Why? Can you explain more? I don't think the protection is needed. The patch is having the following lines::
I remember I suggested you to use datatype = 'letter'. But I realized today that this was supported only for the old sage words objects saved in the pickle jar. So, by introducing it now in this patch, we would have to keep it. So we need to make a decision if we keep it or not. If we don't keep it, you can do the following instead:
|
comment:23
Here it is. I had to change the result to
for the doctest to pass. Please review. |
comment:24
Some last docstring comments:
Best, |
comment:25
New patch, with previous points corrected. Please review. |
comment:26
Vincent, do you think that this ticket is ready to go ? |
Changed keywords from none to sturmian word, tangent word |
Changed author from Thierry Monteil to Thierry Monteil, Frédéric Chapoton |
Reviewer: Vincent Delecroix |
comment:27
Yes it is. Thanks Frédéric for the finalization ! |
Changed reviewer from Vincent Delecroix to Alexandre Blondin Massé, Sébastien Labbé, Vincent Delecroix |
comment:30
Please clarify which patch(es) must be applied. |
This comment has been minimized.
This comment has been minimized.
comment:33
There is a problem building the documentation:
|
comment:34
Attachment: trac_9777-sturm-review-fc.patch.gz I have corrected the doc, back to positive review |
Merged: sage-5.9.beta1 |
Add 3 methods to
sage/combinat/words/finite_word.py
:Add a protecting
is_tangent
method tosage/combinat/words/paths.py
.Depends on #8739.
Apply:
CC: @sagetrac-sage-combinat @sagetrac-abmasse @seblabbe
Component: combinatorics
Keywords: sturmian word, tangent word
Author: Thierry Monteil, Frédéric Chapoton
Reviewer: Alexandre Blondin Massé, Sébastien Labbé, Vincent Delecroix
Merged: sage-5.9.beta1
Issue created by migration from https://trac.sagemath.org/ticket/9877
The text was updated successfully, but these errors were encountered: