Opened 10 years ago
Closed 7 years ago
#14683 closed enhancement (fixed)
New datastructure for iet
Reported by:  vdelecroix  Owned by:  sagecombinat 

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  combinatorics  Keywords:  iet, flat surfaces 
Cc:  Fougeroc  Merged in:  
Authors:  Reviewers:  Frédéric Chapoton  
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
It was decided to develop flatsurf as an independent package
Old description
In #8386 we decided to only move iet to sage/dynamics. This ticket stands for the creation of the new datatype and the correction of the following bug
sage: p = iet.Permutation('a b','b a') sage: q = iet.Permutation('b a','a b') sage: p == q True
Change History (26)
comment:1 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:2 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:3 Changed 9 years ago by
Branch:  → u/vdelecroix/14683iet_new_datastructure 

Cc:  Fougeroc added 
Commit:  → 530fe858e831dc986235125a0baa3630bbc58505 
Keywords:  iet flat surfaces added 
comment:4 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:5 Changed 9 years ago by
Branch:  u/vdelecroix/14683iet_new_datastructure → public/ticket/14683 

Commit:  530fe858e831dc986235125a0baa3630bbc58505 → 4d69202504be026bb76ced36c68c96d2f2da9baa 
rebased. Is this needs review ?
New commits:
4d69202  Merge branch 'u/vdelecroix/14683iet_new_datastructure' of ssh://trac.sagemath.org:22/sage into 14683

comment:6 Changed 9 years ago by
Hi Frederic,
Thanks for the update. It would be nice if you provide clearer message with your commit. The following is completely meaningless
Merge branch 'u/vdelecroix/14683iet_new_datastructure' of ssh://trac.sagemath.org:22/sage into 14683
Vincent
comment:7 Changed 9 years ago by
This was just a rebase, as explained in the comment. Once again, is this ticket in "needs review" ?
comment:8 Changed 9 years ago by
Status:  new → needs_review 

Hi Frederic,
Your comment on trac will not appear in the git history. So please, set your message to "rebase #14683 on sageX.betaY" or something meaningful. The fact that it is "just a rebase" is not an argument to have an ugly commit message.
And yes it is in needs review.
Vincent
comment:9 Changed 9 years ago by
Commit:  4d69202504be026bb76ced36c68c96d2f2da9baa → 7fe873450d232ad485a650e3bb5c701456c6a52f 

Branch pushed to git repo; I updated commit sha1. New commits:
7fe8734  trac #14683 pour raise statements into py3 syntax

comment:10 Changed 9 years ago by
Commit:  7fe873450d232ad485a650e3bb5c701456c6a52f → 9464f5cff7685334f8298e300f46f20c6738dec3 

comment:11 Changed 9 years ago by
Commit:  9464f5cff7685334f8298e300f46f20c6738dec3 → cf153d84fd9539520d1374b6fe2f98bc3ec93f93 

comment:12 Changed 9 years ago by
Hello Vincent,
I am in the middle of the process of reading quickly the code, making small cosmetic changes, like putting doctest continuation and raise statements to new standards, etc. Once done, I will run the tests.
If the tests pass, I could give a positive review, but without checking the new data structure. would it be ok to you ? Have you used this branch heavily, so that you know that it works (and is really better than the current state) ?
comment:13 Changed 9 years ago by
Commit:  cf153d84fd9539520d1374b6fe2f98bc3ec93f93 → db9430e3dfa371ac37f9842974d3fc3e7d35f3fa 

comment:14 Changed 9 years ago by
Milestone:  sage6.3 → sage6.4 

comment:15 followup: 16 Changed 8 years ago by
Status:  needs_review → needs_info 

What's your justification for removing doctests like
TESTS:: sage: t(3/2) Traceback (most recent call last): ... ValueError: value must positive and smaller than length
and replacing the raise ValueError
with an assert
?
(note that I don't really care about this ticket, I am just looking for abuses of assert
)
comment:16 Changed 8 years ago by
Replying to jdemeyer:
What's your justification for removing doctests like
TESTS:: sage: t(3/2) Traceback (most recent call last): ... ValueError: value must positive and smaller than lengthand replacing the
raise ValueError
with anassert
?
Bad design choice ;P
This ticket is pending since it needs some more intensive testing/proof of concepts.
Vincent
comment:17 Changed 8 years ago by
Status:  needs_info → needs_work 

comment:18 Changed 8 years ago by
Commit:  db9430e3dfa371ac37f9842974d3fc3e7d35f3fa → 62797e9d80ee0aa3ef22cf87f7cf65725ef8c916 

comment:19 Changed 8 years ago by
Milestone:  sage6.4 → sage6.9 

comment:20 Changed 7 years ago by
Commit:  62797e9d80ee0aa3ef22cf87f7cf65725ef8c916 → 2b2d683ec628daf2acebe7cd589731535c2a5758 

Branch pushed to git repo; I updated commit sha1. New commits:
2b2d683  Merge branch 'public/ticket/14683' into 7.2.rc0

comment:21 Changed 7 years ago by
Milestone:  sage6.9 → sage7.2 

comment:22 Changed 7 years ago by
Authors:  Vincent Delecroix 

Branch:  public/ticket/14683 
Commit:  2b2d683ec628daf2acebe7cd589731535c2a5758 
Description:  modified (diff) 
Milestone:  sage7.2 → sageduplicate/invalid/wontfix 
Status:  needs_work → needs_review 
comment:23 Changed 7 years ago by
ok, I agree this could be closed. But maybe there would be a short way to solve the bug ?
comment:24 Changed 7 years ago by
I am focusing on #20695 and will not fix this bug in the mean time.
comment:25 Changed 7 years ago by
Reviewers:  → Frédéric Chapoton 

Status:  needs_review → positive_review 
ok, then
comment:26 Changed 7 years ago by
Resolution:  → fixed 

Status:  positive_review → closed 
New commits:
Initial commit: new datastructure for iet.