-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Interval exchange transformations #7145
Comments
comment:1
Attachment: trac_7145-iet-vd.patch.gz |
This comment has been minimized.
This comment has been minimized.
comment:3
First I must say that I am not expert at all in that field. I just know some of Interval exchange transformations. So, Vincent, I read through your patch today and I will post some small corrections in a patch soon. I have also some comments (sometimes suggestions open to you) below : I would gather those related functionalities under a same bag where the user could found them (and all of them) more easily. For example.
or maybe
For
For
I would like the following to work :
where labels of the intervals are facultative:
Why do the following doesn't work:
For template.py : I am getting the following :
|
comment:4
I submitted before preview. So I repeat below the lists that I messed up above. For constructor.py, I suggest to
For iet.py
|
Applies over the precedent patch. |
comment:5
Attachment: trac_7145-review-sl.patch.gz Vincent, I tried your code again with Thierry Monteil who knows better this field and he liked all the functions you implemented. He suggested that you change Permutation* to Also, I notice the following. I think it should be a method and not an attribute :
|
comment:6
Replying to @seblabbe:
It won't be possible. I consider two kinds of permutations
A ReducedPermutation can be identified with a permutation of an ordered alphabet.
The resason is because it's possible to change the alphabet |
This comment has been minimized.
This comment has been minimized.
comment:7
done. Everything is in constructor.py which is imported as iet. Then the iet. gives the (approximately) the following list
done
done
I don't know... the strata here means strata of Abelian differentials on Riemann surfaces. The fact is we need them to understand precisely the structure of Rauzy diagram. It is hence related to Rauzy diagram but somewhat independent of the theory of interval exchange transformations.
done. In fact, it's implemented in .normalize()
done. But a little todo remains. I choose to create canonic labels from the labels of the two iets and this force a conversion of labels to strings. It's not so beautiful if we do not need the labels.
done.
done. |
Attachment: trac_7145-corrections-vd.patch.gz |
comment:8
Dear Vincent, Thanks for doing all those changes. I am now looking at your recent patch and I have a few more comments. I am sorry I was not able to tell them before. I will try now to make a complete review with all my (hopefully) final remarks. After those FIVE remarks answered, I think we will be very near of a positive review! Sébastien ONE.
I am not sure this is a good reason for using an attribute instead of a method. See the following example :
TWO. The documentation and doctest coverage is very good (96%) but not perfect :
Moreover, While you are at it, you may consider to add some INPUT and OUTPUT where there are missing. See the discussion sage-devel: input and output in docstrings THREE. I am getting 2 doctests failures :
FOUR. I use sage-4.2 and I have problem to docbuild a branch of sage. It docbuilds the sage main branch, so I am not able to test if there are no Sphinx warnings and if everythings looks good on the html doc. I am waiting sage-4.3 to come out to test your patch on it and see the doc. FIVE. I changed a little bit the |
Attachment: trac_7145-review2-sl.patch.gz Applies over the precedent 3 patches. |
comment:9
Replying to @seblabbe:
Thanks for this nice review ! It will be a very good patch.
It works now as
up to 100% now
lot of INPUT/OUTPUT added
corrected
I realy agree. In France, we use the coma instead of points to distinguish the integer part and the decimal that why we use dot comman for intervals... I keep it for the french translation ;-) |
Applies over the 4 precedings |
Reviewer: Sebastien Labbe |
comment:10
Attachment: trac_7145-corrections2-vd.patch.gz Great. Point ONE above was addressed. The documentation coverage is now 100% perfect. All tests passed. I builded the documentation and there was several sphinx issues. I corrected them in a patch that I will upload shortly. |
Attachment: trac_7145-review3-sl.patch.gz Applies over all the precedent patches. |
comment:11
Vincent, can you review my last patch and make sure everything is OK with the sphinx output? My patch solved some sphinx syntax, but I feel like more improvements could be done. |
comment:12
Hi,
I'm not sure about what you did with the OUTPUT part of the documentation string. It does not correspond to what is written in the "developer's guide"
What do I do for this ? I will make a complete review of the documentation after the answer. |
comment:13
My fault. In fact, I should update the way I write the OUTPUT part. I will take a closer look to the developer's guide. Feel free to change those. |
comment:14
I made a complete review of the doc. There is no warning when building the documentation and there is INPUT and OUTPUT fields where there are needed. I replace your patch trac_7145-review3-sl.patch with my trac-7145-documentation-review-vd.patch (because I find it more natural to erase the OUTPUT field modification than modify it back). I also made little modifications on default argument on some method (.flips() and .list() of FlippedPermutation) and a change of name (.rauzy_1n() becomes .cylindric()). The latter is due to a forthcoming paper. |
Attachment: trac_7145-documentation-review-vd.patch.gz applies over (trac_7145-iet-vd.patch, trac_7145-review-sl.patch, trac_7145-corrections-vd.patch, trac_7145-review2-sl ,trac_7145-corrections2-vd.patch) |
Attachment: trac_7145-documentation-review-vd.2.patch.gz applies over (trac_7145-iet-vd.patch, trac_7145-review-sl.patch, trac_7145-corrections-vd.patch, trac_7145-review2-sl ,trac_7145-corrections2-vd.patch) |
Attachment: trac_7145-iet-final.patch.gz Apply only this one. |
comment:15
I folded the relevant patches together : trac_7145-iet-final.patch . Positive review. |
comment:16
I just want to add that I tested it on a fresh Sage-4.3. |
Merged: sage-4.3.1.rc0 |
This module implement Interval exchange transformations (iet) (and linear involutions (li)) from a combinatorial point of vue. It also makes the link with strata of Abelian differentials on Riemann surfaces. The three main objects defined in this module are:
There are different class of permutations associated to iet, but all are constructed within a class factory:
The object which links those permutations to the dynamic (Teichmüller flow) of strata of differentials is the Rauzy diagram:
CC: @seblabbe
Component: combinatorics
Author: Vincent Delecroix
Reviewer: Sebastien Labbe
Merged: sage-4.3.1.rc0
Issue created by migration from https://trac.sagemath.org/ticket/7145
The text was updated successfully, but these errors were encountered: