Opened 5 years ago

Last modified 4 years ago

#20154 needs_work task

train-tracks

Reported by: dbenielli Owned by:
Priority: major Milestone: sage-8.0
Component: group theory Keywords: free-group automorphism
Cc: tscrim, tmonteil, vdelecroix Merged in:
Authors: Dominique Benielli, Thierry Coulbois Reviewers:
Report Upstream: N/A Work issues:
Branch: public/train-track (Commits, GitHub, GitLab) Commit: dcc540c770298bac710358c72e35c9ea3961bcde
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

We propose to implement in Sage the train-tracks package developed by Thierry Coulbois:

The main feature and the main achievement of the program is to compute train-track representative for (outer) automorphisms of free groups. phi.train track() computes a train-track representative for the (outer) automorphism phi. This train-track can be either an absolute train-track or a relative train-track. The celebrated theorem of Bestvina and Feighn assures that if phi is fully irreducible (iwip), then there exists an absolute train-track representing phi. The train-track(relative=False) method will terminate with either an absolute train-track or with a topological representative with a reduction: an invariant strict subgraph with non-trivial fundamental group. One more feature of train-tracks (absolute or relative) is to lower the number of Nielsen paths. Setting the stable=True option will return a train-track with at most one indivisible Nielsen path (per exponential stratum if it is a relative train-track).

See also:

Change History (111)

comment:1 Changed 5 years ago by dbenielli

  • Authors set to Dominique Benielli and Thierry Coulbois
  • Component changed from PLEASE CHANGE to combinatorics
  • Description modified (diff)
  • Keywords free-group automorphisme added
  • Type changed from PLEASE CHANGE to task

comment:2 Changed 5 years ago by tscrim

  • Cc tscrim added
  • Description modified (diff)
  • Keywords automorphism added; automorphisme removed

Let me know if you have any questions or if there is anything I can do to help.

In case you were unaware, you might also be interested in that Sage has an implementation of right-angled Artin groups.

comment:3 Changed 5 years ago by tmonteil

  • Cc tmonteil added

comment:4 follow-up: Changed 5 years ago by mmarco

I could be interested in this. Specially to add it as a functionality for Braid groups.

I have taken a quick look at Coulbois code, and it seems that it was writen before we had free groups in sage (or at least, he writes his own implementation of free groups).

So my impression is that we would need to heavily adapt his code in order to fit our current infrastructure.

If you are ok with it, we can start discussing which should be the right structure to follow.

comment:5 in reply to: ↑ 4 Changed 5 years ago by dbenielli

Replying to mmarco:

I could be interested in this. Specially to add it as a functionality for Braid groups.

I have taken a quick look at Coulbois code, and it seems that it was writen before we had free groups in sage (or at least, he writes his own implementation of free groups).

So my impression is that we would need to heavily adapt his code in order to fit our current infrastructure.

If you are ok with it, we can start discussing which should be the right structure to follow.

Hi,

Thanks for your comment. We are still working on train-track adaptation for sage, format code, checked test , that takes more time that I expected were I opened the ticket ... Indeed our main issue is the FreeGroup? implementation (we have already do some change comparing to the train-track code of Thierry) but we have still a reduced FreeGroup?, and the compatibility with the FreeGroup? is Sage is not easy (same use functionality). We of course ok to start discussing about this issue and we wish to find a light solution. The easiest way would be to rename our FreeGroup? but may be we can make something better. So we are open to any suggestion. Dominique

comment:6 Changed 5 years ago by mmarco

Would it be possible to use sage's FreeGroup? (maybe adding to it the functinalities that you need?)

comment:7 follow-up: Changed 5 years ago by tscrim

Let me change the question slightly, what functionality from a free group do you need? (Also, you won't need to rename your free group because it is in a separate module.)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by coulbois

Hello the Sage community,

I finally got a trac.sagemath account to reply !

Of course we need to use (and adapt) the existing FreeGroup? in Sage.

The main difference is my use of alphabets with inverse letters (AlphabetWithInverses? in my program). There have been some discussions with Vincent Delecroix on my choices. I hope my code is not too poorly written and I can adapt easily.

The point is that my free groups are to be used as domain of WordMorphisms? and as such they should have alphabets (and not only generators)... The elements should be words.

May be we could discuss that by mail with Vincent, Thierry M. and Marco ?

comment:9 in reply to: ↑ 8 Changed 5 years ago by tmonteil

Replying to coulbois:

May be we could discuss that by mail with Vincent, Thierry M. and Marco ?

I think this thread/ticket is a good place as it allows other interested people to follow and join the discussion (and does not perturb others).

comment:10 Changed 5 years ago by mmarco

I also would prefear the discussion to happen here. But if it becomes useful at some point, we could try some kind of live meeting (via google hangouts or other similar means)

I am still trying to understand your code to see how could it fit with the existing sage classes.

comment:11 follow-up: Changed 5 years ago by tscrim

It is also entirely possible we will need to abstract out code from the current FreeGroup, IndexedFreeGroup, and your implementation (and from a quick look-over, that is probably what will happen).

comment:12 in reply to: ↑ 11 Changed 5 years ago by coulbois

Yes of course we can also adapt the existing FreeGroup? to my code. For that I need to know how much it is used. In particular as it seems to be only a wrapper of GAP.

Apparently there are two different approaches:

1/ The present FreeGroup? (from GAP) focuses on the fact that free groups are algebraic objects (they have generators and the elements are somehow abstract group elements)

2/ Whereas my approach is closer to that of Words: free group are combinatorial objects and their elements are words on an alphabet.

I don't know how much these two approaches are compatible. And how much we can merge or converge to a common approach. The differences are probably similar to that between FreeMonoid? and Words.

Marco can you tell us how much your FreeGroup? is used, how much it is important that its elements are from a GAP wrap class (ElementLibGAP) ?

comment:13 Changed 5 years ago by tscrim

I have to run now, but a quick response. IIRC, it is used to construct the free algebra, but I believe you're right in that it is basically a wrapper of GAP. I'd have to double-check the latter.

comment:14 Changed 5 years ago by mmarco

Sage's FreeGroup? implementation is mostly a wrapper around GAP's. I think we should keep that since it is the basis for FinitelyPresentedGroups?, and BraidGroup?'s which are also wrapped around GAP.

Elements of a FreeGroup? are represented internally as a Tietze list, so it should be easy to translate from that representation to the one based in words and back (as long as the names of the generators is kept clear). So, instead of rewriting the implementation, I would vote for implementing that translation (which could be interesting in itself), and use that to call your code.

comment:15 Changed 5 years ago by mmarco

By the way, I got an error when trying to test your code:

sage: from all import *
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-1-9796be731807> in <module>()
----> 1 from all import *

/home/mmarco/sage-train-track/all.py in <module>()
----> 1 from free_group import FreeGroup
      2 from free_group_automorphism import FreeGroupMorphism, FreeGroupAutomorphism, free_group_automorphisms
      3 from inverse_alphabet import AlphabetWithInverses

/home/mmarco/sage-train-track/free_group.py in <module>()
     13     from sage.combinat.words.words import FiniteWords_over_OrderedAlphabet as FiniteWords
     14 
---> 15 from inverse_alphabet import AlphabetWithInverses, AbstractAlphabetWithInverses
     16 from free_group_word import FreeGroupWord
     17 

ImportError: cannot import name AbstractAlphabetWithInverses

comment:16 Changed 5 years ago by dbenielli

  • Branch set to u/dbenielli/train_tracks

comment:17 Changed 5 years ago by mmarco

  • Commit set to 8e7f6300f59a3eca9d69233a52fc09b025cb33a2

Did you run the testsuite in this branch?


New commits:

8e7f630first v of train track

comment:18 Changed 5 years ago by dbenielli

Yes, all tests passed

sage: from sage.combinat.words.all import * sage: AlphabetWithInverses?(2) Alphabet with inverses on ['a', 'b']

comment:19 Changed 5 years ago by jhpalmieri

This is in response to http://ask.sagemath.org/question/33238/unable-to-build-the-doc-reference-html/. There are a lot of problems with the formatting of the docstrings. Here are a few fixes as illustrations. See also http://www.sphinx-doc.org/en/stable/rest.html and http://docutils.sourceforge.net/rst.html for the basics of reStructuredText, and http://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings for Sage-specific instructions.

  • src/sage/combinat/words/convex_core.py

    diff --git a/src/sage/combinat/words/convex_core.py b/src/sage/combinat/words/convex_core.py
    index 0183e0f..a3c873a 100644
    a b AUTHORS: 
    66
    77- Thierry COULBOIS (2013-01-01) : initial version
    88- Dominique BENIELLI (2016-02_15) :
    9 AMU University <dominique.benielli@univ-amu.fr>,
    10 Integration in SageMath
     9  AMU University <dominique.benielli@univ-amu.fr>,
     10  Integration in SageMath
    1111
    1212EXAMPLES::
    1313
    class ConvexCore(): 
    4646    Guirardel's convex core of two simplicial
    4747    trees with an action of a free group.
    4848
    49     Let T1 and T2 be trees with actions of the free group FN. G1=T1/FN
    50     and G2=T2/FN are MarkedGraph.
     49    Let `T_1` and `T_`2` be trees with actions of the free group `FN`. `G_1=T_1/FN`
     50    and `G_2=T_2/FN` are MarkedGraph.
    5151
    5252    A ConvexCore is a CW-complex of dimension 2. 2-cells are
    5353    squares. 1-cells are edges labeled by edges of G1 or G2. A square
    5454    is of the form
    5555
     56    ::
     57
    5658          e
    5759        ----->
    5860       |      |
    class ConvexCore(): 
    8991
    9092    EXAMPLES::
    9193
    92     sage: phi=FreeGroupAutomorphism("a->ab,b->ac,c->a")
    93     sage: phi=phi*phi
    94     sage: C=ConvexCore(phi)
    95     sage: print C.slice('c',0)
    96     Looped multi-digraph on 2 vertices
     94        sage: phi=FreeGroupAutomorphism("a->ab,b->ac,c->a")
     95        sage: phi=phi*phi
     96        sage: C=ConvexCore(phi)
     97        sage: print C.slice('c',0)
     98        Looped multi-digraph on 2 vertices
    9799
    98     sage: C.vertices()
    99     [0, 1, 2, 3]
     100        sage: C.vertices()
     101        [0, 1, 2, 3]
    100102
    101     sage: C.squares()
    102     [[3, 0, 2, 1, 'c', 'a']]
     103        sage: C.squares()
     104        [[3, 0, 2, 1, 'c', 'a']]
    103105   
    104     sage: C.twice_light_squares()
    105     [[1, 4, 0, 5, 'a', 'c']]
     106        sage: C.twice_light_squares()
     107        [[1, 4, 0, 5, 'a', 'c']]
    106108
    107109    AUTHORS:
    108110
    class ConvexCore(): 
    686688        - a homotopy equivalence
    687689
    688690        - that maps the root v0 of ``G0.spanning_tree()`` to the root v1 of
    689         ``G1.spanning_tree()``
     691          ``G1.spanning_tree()``
    690692       
    691693        - the image of each vertex has at least two gates.
    692694
    class ConvexCore(): 
    771773
    772774
    773775        - The boundary of a square is a list [e0,e1,e2,e3] of edges such that
    774         e0=(w,v,(a,0)) and e2 are edges with a positive letter
    775         a, and e1=(w,v,(b,1)) and e3 are edges with b a
    776         positive letter.
     776          e0=(w,v,(a,0)) and e2 are edges with a positive letter
     777          a, and e1=(w,v,(b,1)) and e3 are edges with b a
     778          positive letter.
    777779
    778780        - The boundary of an edge it is the list [v0,v1] of the initial vertex
    779         v0=(w,v) followed by the terminal vertex.
     781          v0=(w,v) followed by the terminal vertex.
    780782
    781783        Whereas for lists:
    782784
    783785        - squares: ``[v0,v1,v2,v3,a,b]`` where v0,v1,v2 and v3 are
    784786          integers standing for vertices and a,b are positive letters
    785           labeling edges of G0 and G1:
     787          labeling edges of G0 and G1::
    786788
    787789                a
    788           v3 ------> v2
    789            ^         ^
    790            |         |
    791            |b        |b
    792            |         |
    793            |    a    |
    794           v0 ------>v1
     790           v3 ------> v2
     791            ^         ^
     792            |         |
     793            |b        |b
     794            |         |
     795            |    a    |
     796           v0 ------>v1
    795797
    796798        - edges: ``[v0,v1,(a,side)]`` where ``v0`` and ``v1`` are
    797799          integers standing for vertices a is a label of the tree on
    class ConvexCore(): 
    799801
    800802        INPUT:
    801803
    802         -``cell``square, an edge or a vertex. Squares are bounded
    803         by four vertices, edges by two vertices.
     804        - ``cell`` square, an edge or a vertex. Squares are bounded
     805          by four vertices, edges by two vertices.
    804806
    805807        OUTPUT:
    806808        The boundary of a cell is the list of vertices bounding it.
    807809
    808810        EXAMPLES::
    809811
    810         sage: phi = FreeGroupAutomorphism("a->ab,b->ac,c->a")**2
    811         sage: C = ConvexCore(phi)
    812         sage: C.boundary((Word('C'), 0, 'c', 'a')) # boundary of a square
    813         [(word: C, 0, ('c', 0)),
    814         (word: , 0, ('a', 1)),
    815         (word: Bc, 0, ('C', 0)),
    816         (word: B, 0, ('A', 1))]
     812            sage: phi = FreeGroupAutomorphism("a->ab,b->ac,c->a")**2
     813            sage: C = ConvexCore(phi)
     814            sage: C.boundary((Word('C'), 0, 'c', 'a')) # boundary of a square
     815            [(word: C, 0, ('c', 0)),
     816            (word: , 0, ('a', 1)),
     817            (word: Bc, 0, ('C', 0)),
     818            (word: B, 0, ('A', 1))]
    817819
    818         sage: C.boundary([3, 0, 2, 1, 'c', 'a']) # boundary of a square
    819         [[3, 0, ('c', 0)], [0, 2, ('a', 1)], [2, 1, ('C', 0)], [1, 3, ('A', 1)]]
     820            sage: C.boundary([3, 0, 2, 1, 'c', 'a']) # boundary of a square
     821            [[3, 0, ('c', 0)], [0, 2, ('a', 1)], [2, 1, ('C', 0)], [1, 3, ('A', 1)]]
    820822
    821         sage: C.boundary((Word('Bc'),0,('C',0))) # boundary of an edge
    822         [(word: Bc, 0), (word: B, 0)]
     823            sage: C.boundary((Word('Bc'),0,('C',0))) # boundary of an edge
     824            [(word: Bc, 0), (word: B, 0)]
    823825
    824         sage: C.boundary([2,1,'C']) # boundary of an edge
    825         [2, 1]
     826            sage: C.boundary([2,1,'C']) # boundary of an edge
     827            [2, 1]
    826828        """
    827829
    828830        if isinstance(cell, tuple):

comment:20 follow-up: Changed 5 years ago by mmarco

Building documentation fails. And I get many doctest failing:

sage -t --warn-long 47.3 src/sage/combinat/words/test_train_track.py # 5 doctests failed sage -t --warn-long 47.3 src/sage/misc/sagedoc.py # 4 doctests failed sage -t --warn-long 47.3 src/sage/categories/groups.py # 2 doctests failed sage -t --warn-long 47.3 src/sage/categories/magmas.py # 13 doctests failed sage -t --warn-long 47.3 src/sage/modules/vector_double_dense.pyx # 1 doctest failed sage -t --warn-long 47.3 src/sage/modules/with_basis/representation.py # 2 doctests failed sage -t --warn-long 47.3 src/sage/groups/braid.py # 27 doctests failed sage -t --warn-long 47.3 src/sage/groups/libgap_wrapper.pyx # 76 doctests failed sage -t --warn-long 47.3 src/sage/groups/finitely_presented.py # 241 doctests failed sage -t --warn-long 47.3 src/sage/groups/group.pyx # 2 doctests failed sage -t --warn-long 47.3 src/sage/groups/free_group.py # 123 doctests failed sage -t --warn-long 47.3 src/sage/groups/libgap_group.py # 11 doctests failed sage -t --warn-long 47.3 src/sage/groups/generic.py # 3 doctests failed

I don't think it is a good idea to override the default FreeGorup? implementation. Many parts of Sage deppend on it. It would make more sense to extend it with the methods that are needed for the rest of your code.

comment:21 Changed 5 years ago by chapoton

  • Authors changed from Dominique Benielli and Thierry Coulbois to Dominique Benielli, Thierry Coulbois

comment:22 Changed 5 years ago by chapoton

please use python3 style of print function everywhere : print(x)

comment:23 in reply to: ↑ 20 Changed 5 years ago by coulbois

Yes, I begin to understand, that I will need to use (and add functionalities) to the current FreeGroup?.

The main difficulty is that I rely heavily on WordMorphism?, in particular for parsing:

FreeGroupAutomorphism?("a->ab,b->aC,c->a")

And for applying or compose morphisms. These are heavily used in my program.

This suggest that I would like that my elements of FreeGroup? are Word. Do you have any objection to FreeGroup? also inheriting from Words ? And FreeGroupElement? from Word ?

I guess that it is not too complicated to add Words and Word methods to your FreeGroup? and FreeGroupElement?. But will that interfere with deeper GAP implementations (I am completly new to GAP)?

Would that be satisfactory to Sage gurus that FreeGroup? are both an algebraic object and a symbolic one ?

Finally there is also a IndexedFreeGroup? class which is not a wrapper of GAP, how does it fit in this discussion ?

I don't think it is a good idea to override the default FreeGorup? implementation. Many parts of Sage deppend on it. It would make more sense to extend it with the methods that are needed for the rest of your code.

comment:24 follow-up: Changed 5 years ago by mmarco

I guess it makes sense to consider FreeGroupElements? as Words, but we should be careful to make sure that the multiple inheritance doesn't break anything. We should also probably rewrite the different constructors.

In any case, we need a class for morphisms of free groups (or more general, finitely presented groups), so it might be a good opportunity to design that infrastructure.

comment:25 in reply to: ↑ 24 Changed 5 years ago by tscrim

Replying to mmarco:

In any case, we need a class for morphisms of free groups (or more general, finitely presented groups), so it might be a good opportunity to design that infrastructure.

This is my opinion as well. Parsing of the input is an API decision and, IMO, not a good reason to use a specific implementation. Unless you are using other features of WordMorphism that are not (currently) available to the current group morphisms or I am misunderstanding comment:23.

Actually, I am somewhat not in favor of using words because of the need to handle inverses in the free group. There should not really be that much of an implementation difference between the free group using words or GAP, just an API difference...at least I'd imagine there shouldn't be...

comment:26 Changed 5 years ago by git

  • Commit changed from 8e7f6300f59a3eca9d69233a52fc09b025cb33a2 to 2fbe7c6745dc59fc60b8ccb1c616224b3c60a5b3

Branch pushed to git repo; I updated commit sha1. New commits:

2fbe7c6improvment for doc build

comment:27 follow-up: Changed 5 years ago by coulbois

Dominique finished to set-up the test and to clean-up the doc. Let's say that we have a clean version 1. I am refering to the master branch of my https://github.com/coulbois/sage-train-track repository.

1/ Please try and explore this version for functionnalities. Do you find all what you need for free group automorphisms and train-tracks ?

2/ Now, we need to make that compatible with the FreeGroup? of sage.groups. I propose to start from the file sage.groups.free_group.py and add the methods I need. This is the purpose of the work branch in my https://github.com/coulbois/sage-train-track repository. I fear that this implies giving-up the fact that free groups elements are words. Everybody is okay with that ?

(I guess I am not in the correct Sage workflow, please comment if you have a better one).

comment:28 Changed 5 years ago by tscrim

I still haven't taken a detailed looked yet (sorry), but this does look very impressive. I am looking forward to getting this project and code included into Sage.

I think it would be good for you to list here exactly what functionality you're missing from free groups in Sage (and their morphisms) and/or to create a separate ticket, which would become a dependency of this ticket, that would implement these features.

Although, you are not really loosing the interpretation of free group elements as words because you have the Tietze(), but it will likely require some changes to the implementations. (Sorry for a bit of repetition, but IMO, a finite word is much more naturally associated with a free monoid because of the concatenation does not involve cancellation.) The biggest possible obstruction to this that I see currently is that you are using some feature only implemented for word morphisms.

Math question, does the theory of train-tracks extend to free groups on (countably) infinite generators?

comment:29 in reply to: ↑ 27 Changed 5 years ago by mmarco

2/ Now, we need to make that compatible with the FreeGroup? of sage.groups. I propose to start from the file sage.groups.free_group.py and add the methods I need. This is the purpose of the work branch in my https://github.com/coulbois/sage-train-track repository. I fear that this implies giving-up the fact that free groups elements are words. Everybody is okay with that ?

I am certainly okay with that. From a quick look at your code, it sounds very natural to rewrite it on top of existing FreeGroups? (for instance, you needed to add some way to concatenate word with cancelations, whereas in the framework of freegroups you have all that for free as the standard product).

(I guess I am not in the correct Sage workflow, please comment if you have a better one).

In theory, we should handle the branches in this server instead of github. But given the size of the project, and the fact that we are in a stage where we are pretty much experimenting before deciding the actual dessign, it may be ok to handle this early branches somewhere else (as long is everybody is ok about working through github). Although at some point we should move it all to trac branches.

comment:30 Changed 5 years ago by dbenielli

Hi all,

As Thierry says previously we decided to make the compatibilyty with the sage FreeGroup? through the hard way, used it and add functionnalities. Three or more module need to be rewrited, when we obtain a first draft version I am pushing in in the sage ticket. So there no need for you to worry about a external branche.

Dominique

comment:31 Changed 5 years ago by git

  • Commit changed from 2fbe7c6745dc59fc60b8ccb1c616224b3c60a5b3 to 3ecdcdfe553c1f97769d69f659977e1b6eebaabb

Branch pushed to git repo; I updated commit sha1. New commits:

3ecdcdfrm fille

comment:32 follow-up: Changed 5 years ago by coulbois

Advice/approval needed

I started using and modifying the existing FreeGroup?. I refer to the work branch of my git repo (not the sage one)

1/ My guess is that I can rely mainly on the Tietze() method of FreeGroupElement?. I have no idea of the GAP implementation of FreeGroupElement?.

2/ I extended init() methods to allow more API (accepting strings for example). I added getitem and comparisons methods. Now FreeGroupElement? behave loosely as words and I expect to rely on them.

3/ I started to adapt the FreeGroupMorphism?. I am not really willing to write a GroupMorphism? (even an abstract one) class as I have no idea of what Group and GroupElement? look like. I propose to use a _morph variable to store a dictionnary that map FreeGroupElement? (the generators and their inverses) to FreeGroupElement? (once again I have no idea of the cost, may be a dictionnary from integers to integer list together with Tietze() would be more efficient though less natural).

The doc is not yet up-to-date.

I am currently working further. But please stop me if you believe my choices are stupid.

comment:33 Changed 5 years ago by git

  • Commit changed from 3ecdcdfe553c1f97769d69f659977e1b6eebaabb to efa12d37913aadd10e7c8cb59e9068aae64e6ff7

Branch pushed to git repo; I updated commit sha1. New commits:

efa12d3remove all

comment:34 in reply to: ↑ 32 Changed 5 years ago by tscrim

Replying to coulbois:

1/ My guess is that I can rely mainly on the Tietze() method of FreeGroupElement?. I have no idea of the GAP implementation of FreeGroupElement?.

This is the way to go because Tietze() pull info from its GAP data. This should also be reasonably fast.

2/ I extended init() methods to allow more API (accepting strings for example). I added getitem and comparisons methods. Now FreeGroupElement? behave loosely as words and I expect to rely on them.

I am okay with the __getitem__. There is a default total order from the generic GAP element implementation, which the total order may or may not be good, but it is likely one of the faster ways to check for (in)equality. However, I haven't looked at your current code yet.

3/ I started to adapt the FreeGroupMorphism?. I am not really willing to write a GroupMorphism? (even an abstract one) class as I have no idea of what Group and GroupElement? look like. I propose to use a _morph variable to store a dictionnary that map FreeGroupElement? (the generators and their inverses) to FreeGroupElement? (once again I have no idea of the cost, may be a dictionnary from integers to integer list together with Tietze() would be more efficient though less natural).

No matter what, I think you would need a FreeGroupMorphism class for everything that you want to do. I think the internal storage method should depend on the application, but the user input should be a tuple, which is consistent with the rest of Sage and requires less validation of the input.

You will probably want to create a FreeGroupHomset (which you will need to add a _hom_ method to FreeGroup to create this), whose elements are your FreeGroupMorphism, which can be constructed directly from the hom method. By having the morphism take a tuple as input, you shouldn't have to modify the hom method.

A question: does your framework handle morphisms to free groups with different generator labels? If not, would it be easy to support this (and do you want to)? If also no, then don't worry about it.

comment:35 Changed 5 years ago by dbenielli

  • Branch changed from u/dbenielli/train_tracks to public/train_track
  • Commit efa12d37913aadd10e7c8cb59e9068aae64e6ff7 deleted

comment:36 Changed 5 years ago by dbenielli

  • Component changed from combinatorics to group theory
  • Milestone changed from sage-7.1 to sage-7.3

comment:37 Changed 5 years ago by vdelecroix

  • Branch changed from public/train_track to public/train-track
  • Cc vdelecroix added
  • Commit set to 9f9d12bb0ec6a7d5c49b1f5bbff95b0e5f28b1b8

New commits:

dce1fc0initial commit for train track for free groups
2f68ae6print p3
8a67358doc
80381b4doc
0e2401ftests passed
02f8668deprecation warning for the old free_group.py file
ea56cefsimpler comparisons
9f9d12bremove trailing whitespaces

comment:38 Changed 5 years ago by vdelecroix

(I fixed the name of the branch, it was train-track and not train_track)

comment:39 Changed 5 years ago by chapoton

For python3 compatibility:

  • please do not use the it.next method, but instead next(it)
  • please do not write print (something) but rather print(something) (no space after print)

see my patchbot's report for more information

comment:40 Changed 5 years ago by git

  • Commit changed from 9f9d12bb0ec6a7d5c49b1f5bbff95b0e5f28b1b8 to 020d0a2b4739c15815da5209044bac01ce4661db

Branch pushed to git repo; I updated commit sha1. New commits:

020d0a2python3 compatibility

comment:41 Changed 5 years ago by dbenielli

ok - use the it.next method, but instead next(it) ok - print(something)

in commit 020d0a2 python3 compatibility

comment:42 Changed 5 years ago by git

  • Commit changed from 020d0a2b4739c15815da5209044bac01ce4661db to 737c4e51196298e5bffbc3a79ca5a0311fd83f34

Branch pushed to git repo; I updated commit sha1. New commits:

737c4e5doc

comment:43 Changed 5 years ago by chapoton

there remains two lines with old-style python2 print (in the doctests) see red plugin in latest patchbot report (which also gives two false positives that can be ignored)

comment:44 Changed 5 years ago by git

  • Commit changed from 737c4e51196298e5bffbc3a79ca5a0311fd83f34 to 25934b5c05c112eaa6c451499a798ac846ba69e0

Branch pushed to git repo; I updated commit sha1. New commits:

25934b5last print phython3 style

comment:45 Changed 5 years ago by git

  • Commit changed from 25934b5c05c112eaa6c451499a798ac846ba69e0 to 7db874b48bed1601bbcb115e307cfbfaabb2fd38

Branch pushed to git repo; I updated commit sha1. New commits:

7db874bconstructions doc

comment:46 Changed 5 years ago by git

  • Commit changed from 7db874b48bed1601bbcb115e307cfbfaabb2fd38 to 0e0d7da551b8539bbb5295d2200dae9bb9497f52

Branch pushed to git repo; I updated commit sha1. New commits:

0e0d7dastill print

comment:47 Changed 5 years ago by git

  • Commit changed from 0e0d7da551b8539bbb5295d2200dae9bb9497f52 to 2d369575b7d45ea77cf0d1c0d3118a7cd9ddd96e

Branch pushed to git repo; I updated commit sha1. New commits:

2d36957construction

comment:48 Changed 5 years ago by dbenielli

  • Status changed from new to needs_review

comment:49 Changed 5 years ago by chapoton

doc does not build, and there remains old-style prints, see my patchbot's latest report

comment:50 Changed 5 years ago by git

  • Commit changed from 2d369575b7d45ea77cf0d1c0d3118a7cd9ddd96e to 496bd4a46d3b257afd62e0dd8ab5ea2d91b36d89

Branch pushed to git repo; I updated commit sha1. New commits:

c390257still print
496bd4adoc

comment:51 Changed 5 years ago by git

  • Commit changed from 496bd4a46d3b257afd62e0dd8ab5ea2d91b36d89 to c3aae43ae3d9a3462592759cd2f5f34c6afb9505

Branch pushed to git repo; I updated commit sha1. New commits:

c3aae43doc

comment:52 Changed 5 years ago by git

  • Commit changed from c3aae43ae3d9a3462592759cd2f5f34c6afb9505 to 45d380b0843995b30c47ebdc054f0dd1bdb9cfda

Branch pushed to git repo; I updated commit sha1. New commits:

45d380bdoc

comment:53 Changed 5 years ago by git

  • Commit changed from 45d380b0843995b30c47ebdc054f0dd1bdb9cfda to af880666e78263ebd519d8c79e4cb2d46cefbeb9

Branch pushed to git repo; I updated commit sha1. New commits:

af88066improve tests coverage

comment:54 Changed 5 years ago by git

  • Commit changed from af880666e78263ebd519d8c79e4cb2d46cefbeb9 to d35c1d2fcbb626332e8ad15a1fc98441edb4b0aa

Branch pushed to git repo; I updated commit sha1. New commits:

d35c1d2tests coverage

comment:55 Changed 5 years ago by vdelecroix

Hello Dominique, Thierry,

There are two current problems (see the failing doctests of patchbot):

  • the comparison between elements of the free group changed. Before we had the following order on generators x_0^-1, x_0, x_1^-1, x_1, ... But with your branch, because you use direct comparison on the Tietze generators, it became x_n^-1, x_{n-1}^-1, ..., x_0^-1, x_0, x_1, ..., I think the former was preferable. What do you think?
  • there are a lot of failure related to the free_group.py being moved. You just need to change the occurrence of sage.groups.free_group by sage.groups.free_groups.free_group everywhere.

comment:56 follow-up: Changed 5 years ago by coulbois

No problem for the order of the generators and their inverses. However the reason for rewriting the _cmp_ method was that I want the neutral element to be smaller than all other elements of the free group. Which was not the case with the previous version.

I do not know how the previous order was implemented (deep inside GAP ?)

comment:57 Changed 5 years ago by tscrim

In the current Sage, it calls the comparison between the (lib)GAP objects; so my understanding is GAP handles the comparison.

comment:58 Changed 5 years ago by dbenielli

  • Commit changed from d35c1d2fcbb626332e8ad15a1fc98441edb4b0aa to 7a7da77df0eac29dc6b5783f023336cdaa44f51d

Hello All,

My last commits do not appear in the ticket comments:

  • I did my best to remove failures that appear in the patchchboot log (the make worked on my computer strange behavior?), and improve tests coverage, that seems to be a blocked point.
  • For the order generators in my sens the remark from Vincent was about the previous version of free_group but the train_track version because a lot of order changed with the use of free_ group generators of Gap. But maybe, I confuse and the re-writting of _cmp method also change the orther.

Best regards Dominique


New commits:

f060b61tests failures correction
7a7da77tests failuures

comment:59 in reply to: ↑ 56 Changed 5 years ago by vdelecroix

Replying to coulbois:

No problem for the order of the generators and their inverses. However the reason for rewriting the _cmp_ method was that I want the neutral element to be smaller than all other elements of the free group. Which was not the case with the previous version.

It was...

sage: F = FreeGroup(['a','b'])
sage: a,b = F.gens()
sage: l = [a,b,~a,~b,a*b,~a*b,F.one()]
sage: sorted(l)
[1, a^-1, a, b^-1, b, a^-1*b, a*b]
sage: F.one() < a
True
sage: a < F.one()
False

What is wrong with the above? It is first sorted by length and then (almost) lexicographically. This is indeed different from order on lists which is straight lexicographic.

I do not know how the previous order was implemented (deep inside GAP ?)

It indeed used the gap comparison which seems reasonable to me.

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:60 Changed 5 years ago by dbenielli

The ticket test failled status comes from a a problem with pathboot see http://ask.sagemath.org/question/33899/doc-test-fail-in-patchboot-and-not-on-my-computer/

comment:61 Changed 5 years ago by git

  • Commit changed from 7a7da77df0eac29dc6b5783f023336cdaa44f51d to c9da8b062929c663c865dbccd254790be7678c7f

Branch pushed to git repo; I updated commit sha1. New commits:

c9da8b0Merge branch 'public/train-track' in 7.3.b9

comment:62 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

coverage is not yet 100%

comment:63 Changed 5 years ago by git

  • Commit changed from c9da8b062929c663c865dbccd254790be7678c7f to 857ff2ccd8cc8f743dbd11c55437ae78b2c02c51

Branch pushed to git repo; I updated commit sha1. New commits:

2c332dftest coverage
857ff2cMerge branch 'public/train-track' of git://trac.sagemath.org/sage into public/train-track

comment:64 Changed 5 years ago by dbenielli

  • Status changed from needs_work to needs_review

The patchboot is now on test passed (TestsPassed? 7.3.rc0 Ubuntu/14.04/x86_64/3.13.0-92-generic/librae), please review our work on train track for its intergration in sage

comment:65 Changed 4 years ago by git

  • Commit changed from 857ff2ccd8cc8f743dbd11c55437ae78b2c02c51 to 8f9c87ceb239c1794029428eb01150cf372a310e

Branch pushed to git repo; I updated commit sha1. New commits:

8f9c87ctry foreign latex failure

comment:66 Changed 4 years ago by tscrim

I do still plan on reviewing this, but I need some things to settle down first before I can do a large-scale code review.

comment:67 follow-up: Changed 4 years ago by vdelecroix

what about 59? I see no need to change _cmp_.

comment:68 Changed 4 years ago by vdelecroix

reference needed is not a valid reference ;-)

comment:69 Changed 4 years ago by vdelecroix

In the documentation of almost all examples of automorphism the information is duplicated as in

This is a pseudo-Anosov mapping class of the 5-punctured
sphere. Thus this is not an iwip. However, its representative
on the rose in train-track.

OUTPUT:

an Automorphism of F_4 given in [BH] see also [Kapovich].
This is a pseudo-Anosov mapping class of the 5-punctured     <-- exact copy!
sphere. Thus this is not an iwip. However, its representative
on the rose in train-track.

comment:70 in reply to: ↑ 67 ; follow-up: Changed 4 years ago by dbenielli

Replying to vdelecroix:

what about 59? I see no need to change _cmp_.

the rewriting of _cmp_ order leads to some modifications outside free_group for tests, but I did changes, so I think is ok unless if there are some GAP reasons.

comment:71 in reply to: ↑ 70 Changed 4 years ago by vdelecroix

Replying to dbenielli:

Replying to vdelecroix:

what about 59? I see no need to change _cmp_.

the rewriting of _cmp_ order leads to some modifications outside free_group for tests, but I did changes, so I think is ok unless if there are some GAP reasons.

This is precisely my question: why is _cmp_ changed at all?

comment:72 Changed 4 years ago by git

  • Commit changed from 8f9c87ceb239c1794029428eb01150cf372a310e to a13b02db866233ef10c92a5670acf4176dc114b7

Branch pushed to git repo; I updated commit sha1. New commits:

a13b02dremove _cmp

comment:73 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Hello,

Commenting is not removing. More importantly, you have to revert all the changes you have done in previous commits related to this change of comparison order.

comment:74 Changed 4 years ago by git

  • Commit changed from a13b02db866233ef10c92a5670acf4176dc114b7 to cd641776132ac1cc90be7e49e717c0101f9042d5

Branch pushed to git repo; I updated commit sha1. New commits:

cd64177remove __cmp and tests induced

comment:75 Changed 4 years ago by coulbois

  • Status changed from needs_work to needs_review

We revert to the _cmp_ method of FreeGroupElement? to that of the GAP-wrapper (meaning that we removed the _cmp_ method from the free_groups/free_group.py file). Warning: this comparison is undocumented and it seems that it first compares the length of reduced words and then sort words of equal length in alphabetic order. In our train-track package it is mainly used while running the Nielsen-Whitehead algorithm to invert automorphisms (see FreeGroupMorphism?.invert()). The order wrapped from GAP is satisfactory in this purpose.

We revert the doc-tests (that we had previously modified) from sage/groups/finitely_presented.py which use this order.

All doc-tests pass.

We now wait for other things to improve according to reviewers.

comment:76 Changed 4 years ago by vdelecroix

Hi Thierry, Hi Dominique,

The behavior is certainly documented in GAP. It might be worth to also have it (and test it) inside Sage. I am looking at the other pieces of your code now.

Vincent

comment:77 follow-up: Changed 4 years ago by chapoton

Do you really need compare_letters ? Recall that cmp(...) is going to disappear in python3.

comment:78 in reply to: ↑ 77 Changed 4 years ago by coulbois

You mean the compare_letters in AlphabetWithInverses? ?

Well, we really need to be carefull on the order of the alphabet. And I don't see a way around that. We need to have alphabets where each letter comes with an inverse, we need to be able to add and remove letters.

I agree that this is inspired by old versions of Words and Alphabet which now have disappeared from Sage.

The AlphabetWithInverses? is now only used by our InverseGraph? (and not by FreeGroup?), and we want to strictly comply with Serre's definition (see Arbres, amlagames, SL2, aka Trees).

comment:79 Changed 4 years ago by chapoton

there is only one compare_letters in the git branch, and it seems not to be used anywhere (only appears in its own doc)

sorting should now generally be done using the keyword key= instead of cmp=

comment:80 Changed 4 years ago by vdelecroix

The compare_letters might be indirectly used in some word algorithm (though I doubt it).

comment:81 Changed 4 years ago by chapoton

there is no compare_letters anywhere else in sage

(git grep "compare_letters")

comment:82 Changed 4 years ago by git

  • Commit changed from cd641776132ac1cc90be7e49e717c0101f9042d5 to 653a2510c2077063e1e0824d247d5d8852792a7a

Branch pushed to git repo; I updated commit sha1. New commits:

653a251compare_letters() removed as it is never used

comment:83 Changed 4 years ago by git

  • Commit changed from 653a2510c2077063e1e0824d247d5d8852792a7a to ae9a96fbbbb800016ca9b0c13e7b77ced98dbab5

Branch pushed to git repo; I updated commit sha1. New commits:

ae9a96fMinor correction in the doc of FreeGroupAutomorphism.train_track() (due to my French mother tongue, more than two meant two or more + correction in the doc of TrainTrackMap.stabilize() to be more clear that the output is the folding map (not the train-track map) + one correction for a case where that method returned False instead of the identity map

comment:84 Changed 4 years ago by git

  • Commit changed from ae9a96fbbbb800016ca9b0c13e7b77ced98dbab5 to d92640a431ce2ef957ca16b4113eb6abf986230d

Branch pushed to git repo; I updated commit sha1. New commits:

d92640aMerge branch 'public/train-track' in 7.5.b2

comment:85 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:86 Changed 4 years ago by dbenielli

I sent a question to https://ask.sagemath.org/question/35988/patchboot-coverage-failure-and-review/ because I do not know what to do to fix it.

Hi sage team, My ticket #20154: train-tracks in public/train-track, is in need-work status, I guess the issue comes from coverage. But I do not see the fie ext/interpreters/wrapper_cc.pyx in my distribution , and for me this file doesn't belong to the ticket (that was true previously whent the patchboot works). I have absolutly no idea how to fix it, could you please tell me what to do to fix it and succes the review.

Thanks Dominique

========== coverage ========== git checkout patchbot/ticket_merged Already on 'patchbot/ticket_merged' Missing doctests in ext/interpreters/wrapper_cc.pyx: 0 / 2 = 0% Full doctests in groups/free_groups/convex_core.py: 18 / 18 = 100% Full doctests in groups/free_groups/free_group.py: 22 / 22 = 100% Full doctests in groups/free_groups/free_group_automorphism.py: 61 / 61 = 100% Full doctests in groups/free_groups/graph_map.py: 18 / 18 = 100% Full doctests in groups/free_groups/graph_self_map.py: 41 / 41 = 100% Full doctests in groups/free_groups/inverse_alphabet.py: 24 / 24 = 100% Full doctests in groups/free_groups/inverse_graph.py: 40 / 40 = 100% Full doctests in groups/free_groups/marked_graph.py: 16 / 16 = 100% Full doctests in groups/free_groups/test_train_track.py: 4 / 4 = 100% Full doctests in groups/free_groups/train_track_map.py: 23 / 23 = 100% Coverage went from 41463 / 43258 = 95.850% to 41711 / 43508 = 95.870%

comment:87 Changed 4 years ago by chapoton

This kind of patchbot report you can safely ignore. The patchbot is not perfect, and sometimes gives wrong failures. Patchbot is only here to help, not to be taken as an absolute requirement.

The problem now is that you need to rebase your branch on the latest beta, because there is a conflict (this make the branch red at top of the present page)

comment:88 Changed 4 years ago by git

  • Commit changed from d92640a431ce2ef957ca16b4113eb6abf986230d to 48df86fc72846c91a5e1082b6cbf4d1ad1fd746b

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3bd8e1ctests failuures
143eb01test coverage
d53d06atry foreign latex failure
8551333remove _cmp
6774b7fremove __cmp and tests induced
283c702compare_letters() removed as it is never used
fd8ddecMinor correction in the doc of FreeGroupAutomorphism.train_track() (due to my French mother tongue, more than two meant two or more + correction in the doc of TrainTrackMap.stabilize() to be more clear that the output is the folding map (not the train-track map) + one correction for a case where that method returned False instead of the identity map
c060ab1small typo
c4a2309fusion
48df86fmerge bug

comment:89 Changed 4 years ago by git

  • Commit changed from 48df86fc72846c91a5e1082b6cbf4d1ad1fd746b to 514cff1e78b99abdb4300362e8828e76cc5f94ac

Branch pushed to git repo; I updated commit sha1. New commits:

514cff1correction for python3

comment:90 Changed 4 years ago by git

  • Commit changed from 514cff1e78b99abdb4300362e8828e76cc5f94ac to d9256f7d18063828a66c3a782698b5a9e389d446

Branch pushed to git repo; I updated commit sha1. New commits:

d9256f7nothing

comment:91 Changed 4 years ago by dbenielli

  • Status changed from needs_work to needs_review

I think I have corrected all python3 troubles, even though the patchboot did not give new result since 2016-12-14, I think it ok for review

comment:92 Changed 4 years ago by chapoton

Because you are not yet trusted, I have to launch my personal bot manually.

Result:

  • some failing doctests
  • some bad unicode character (maybe add uft8 declaration at top of file ?)
  • need to replace <type 'str'> by <... 'str'> for python3 compatibility

see patchbot report for more

comment:93 Changed 4 years ago by git

  • Commit changed from d9256f7d18063828a66c3a782698b5a9e389d446 to 9f3c92f786a99d2e4d38cf650d43b26327a1b472

Branch pushed to git repo; I updated commit sha1. New commits:

9f3c92fpthshoot coorected

comment:94 Changed 4 years ago by jhpalmieri

You should rebase this on the most recent version of Sage: I think the doctest failures are because of new code that uses free groups and imports it from its old location, leading to deprecation warnings.

comment:95 Changed 4 years ago by dbenielli

Many thanks for your help, I corrected the bad UTF8 unicode from python 2. And its seems to work with the new rebase version too. I cross fingers my last commit pass the pathboot, and the ticket can pass.

comment:96 Changed 4 years ago by chapoton

  • Milestone changed from sage-7.3 to sage-8.0
  • Status changed from needs_review to needs_work

does not apply

comment:97 Changed 4 years ago by git

  • Commit changed from 9f3c92f786a99d2e4d38cf650d43b26327a1b472 to 125259102f7f65182ed3d7acda07b8fae016a2a5

Branch pushed to git repo; I updated commit sha1. New commits:

0bd5e41Merge branch 'public/train-track' of git://trac.sagemath.org/sage into public/train-track
1252591Doing some doc improvements.

comment:98 Changed 4 years ago by tscrim

I've done the rebase to the current Sage version and did a little bit of doc cleanup and other small tweaks. It still needs a complete doc readthrough (I maybe got through 30%). There are some references that seem like they needed to be added.

comment:99 Changed 4 years ago by dbenielli

Hi,

Thanks for your update for the new version 8, Unfortunately my version obtained by pull is not working, I do not understand what happens.

Otherwise I would like to succes the review, and I would know what is missing, and I can do.

Thanks for help

Dominique

PS error message of doc build

Error building the documentation. Traceback (most recent call last):

File "/home/dominique/projets/sage/local/lib/python2.7/runpy.py", line 174, in _run_module_as_main

"main", fname, loader, pkg_name)

File "/home/dominique/projets/sage/local/lib/python2.7/runpy.py", line 72, in _run_code

exec code in run_globals

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/main.py", line 2, in <module>

main()

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/init.py", line 1640, in main

builder()

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/init.py", line 510, in _wrapper

build_many(build_ref_doc, L)

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/init.py", line 266, in build_many

results.append(target(arg))

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/init.py", line 70, in build_ref_doc

getattr(ReferenceSubBuilder?(doc, lang), format)(*args, kwds)

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/init.py", line 720, in _wrapper

getattr(DocBuilder?, build_type)(self, *args, kwds)

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/init.py", line 104, in f

runsphinx()

File "/home/dominique/projets/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 215, in runsphinx

raise exception

OSError: [manifolds] /home/dominique/projets/sage/src/doc/en/reference/manifolds/index.rst:4: WARNING: undefined label: tensors-on-free-modules (if the link has no caption the label must precede a section header)

comment:100 Changed 4 years ago by tscrim

Try running make doc-clean && make.

One of the most tedious things is to bring the doc formating similar to the changes I made (see also the sage dev guide). I will try to go through the code itself soon too.

comment:101 Changed 4 years ago by git

  • Commit changed from 125259102f7f65182ed3d7acda07b8fae016a2a5 to 747f50449e955cc56ad13b846118556dd2c43960

Branch pushed to git repo; I updated commit sha1. New commits:

747f504minor typos in docstring corrected

comment:102 Changed 4 years ago by dbenielli

Many thanks you very munch for your upgrade,

the make doc-clean && make is now ok

Do you think we can go back on need review status?

comment:103 Changed 4 years ago by tscrim

Someone still needs to go through and standardize the docstrings to use Sage's conventions (comment:100). In addition to the changes I made, you can also use the Sage conventions page, although the INPUT: items should not end in periods/full stops/. (unless they are really long) and you do not need an INPUT: block if there is no input (self does not count) or an OUTPUT: if the output is clear from the 1-line doc. I will go back to looking through the code once that is done.

This also needs a (likely trivial) rebase on the latest develop version.

Last edited 4 years ago by tscrim (previous) (diff)

comment:104 Changed 4 years ago by git

  • Commit changed from 747f50449e955cc56ad13b846118556dd2c43960 to 3a4bf05c356506567cf3e84fd7a9402b6960d71a

Branch pushed to git repo; I updated commit sha1. New commits:

3a4bf05minor typo

comment:105 Changed 4 years ago by git

  • Commit changed from 3a4bf05c356506567cf3e84fd7a9402b6960d71a to c1805dbcd2b10672344b1bb2f474f01a97500fa0

Branch pushed to git repo; I updated commit sha1. New commits:

c1805dbmerge with develop branch

comment:106 Changed 4 years ago by git

  • Commit changed from c1805dbcd2b10672344b1bb2f474f01a97500fa0 to b81e62dfbb002bf48688c259005f2875fc66727b

Branch pushed to git repo; I updated commit sha1. New commits:

b81e62dINPUT end . correction

comment:107 Changed 4 years ago by dbenielli

Hi,

I done the rebase and the correction for INPUT intems end with . only where there are long. Tell me if it is ok for a need review now

Thanks Dominique

comment:108 Changed 4 years ago by tscrim

Still a lot of doc issues (see also comment:19):

  • You need to replace $ with `.
  • What does \index{Nielsen path} do? I think it will not do what you want with the build documentation.
  • Are the macros \CVN and \FN defined? If not, you probably want to replace them with the corresponding true Latex code (rather than add to the macro list).
  • Use \ZZ instead of \mathbb Z.
  • References should go in the master reference file doc/en/reference/references/index.rst and have more descriptive names.
  • Format like:
    def foo(self):
        """
        Short description.
    
        ...
        """
    
  • Your ascii art square for ConvexCore should be is of the form:: (with a double colon). Although having spaces here will be better than the .. The doc should not have any trouble getting this to look nice.
  • There are a number of variables that should be either formatted for latex (`x`) or code (``x``). In particular, things like ``False`` should be code formatted.
  • It is better to put inputs for __init__ in the class-level docstring so it will be visible.

Some code issue I noticed on a quick readthrough:

  • It would be good to provide some sort of deprecation warnings for all non-hidden functions/classes (i.e., not _lexi_gen). See the deprecation option of lazy_import.
  • Use new-style Python clases: class ConvexCore(object):. Although can the user ever see it? If so, perhaps it should inherit from SageObject.
  • Never check things like if x is True: (resp. False:); simply do if x: (resp. if not x:).
  • It is faster to check if todo: instead of if len(todo) > 0.
  • Instead of __str__, if it is a SageObject? subclass, you should use _repr_ (e.g., subclasses of DiGraph). Otherwise, we prefer to use __repr__.
  • groups/free_groups/test_train_track.py might be better as tests/train_tracks.py.

comment:109 Changed 4 years ago by tscrim

However, I will start going through the code part soon.

comment:110 Changed 4 years ago by git

  • Commit changed from b81e62dfbb002bf48688c259005f2875fc66727b to 573701b06cccc7e2fa3982534e86cc4e8d8a990b

Branch pushed to git repo; I updated commit sha1. New commits:

573701bsome correction from tscrim comments

comment:111 Changed 4 years ago by git

  • Commit changed from 573701b06cccc7e2fa3982534e86cc4e8d8a990b to dcc540c770298bac710358c72e35c9ea3961bcde

Branch pushed to git repo; I updated commit sha1. New commits:

dcc540csome correction from tscrim comments
Note: See TracTickets for help on using tickets.