Opened 8 years ago
Closed 5 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 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 7 years ago by
 Branch set to u/vdelecroix/14683iet_new_datastructure
 Cc Fougeroc added
 Commit set to 530fe858e831dc986235125a0baa3630bbc58505
 Keywords iet flat surfaces added
comment:4 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:5 Changed 7 years ago by
 Branch changed from u/vdelecroix/14683iet_new_datastructure to public/ticket/14683
 Commit changed from 530fe858e831dc986235125a0baa3630bbc58505 to 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 7 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 7 years ago by
This was just a rebase, as explained in the comment. Once again, is this ticket in "needs review" ?
comment:8 Changed 7 years ago by
 Status changed from new to 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 7 years ago by
 Commit changed from 4d69202504be026bb76ced36c68c96d2f2da9baa to 7fe873450d232ad485a650e3bb5c701456c6a52f
Branch pushed to git repo; I updated commit sha1. New commits:
7fe8734  trac #14683 pour raise statements into py3 syntax

comment:10 Changed 7 years ago by
 Commit changed from 7fe873450d232ad485a650e3bb5c701456c6a52f to 9464f5cff7685334f8298e300f46f20c6738dec3
comment:11 Changed 7 years ago by
 Commit changed from 9464f5cff7685334f8298e300f46f20c6738dec3 to cf153d84fd9539520d1374b6fe2f98bc3ec93f93
comment:12 Changed 7 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 7 years ago by
 Commit changed from cf153d84fd9539520d1374b6fe2f98bc3ec93f93 to db9430e3dfa371ac37f9842974d3fc3e7d35f3fa
comment:14 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:15 followup: ↓ 16 Changed 6 years ago by
 Status changed from needs_review to 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 in reply to: ↑ 15 Changed 6 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 6 years ago by
 Status changed from needs_info to needs_work
comment:18 Changed 6 years ago by
 Commit changed from db9430e3dfa371ac37f9842974d3fc3e7d35f3fa to 62797e9d80ee0aa3ef22cf87f7cf65725ef8c916
comment:19 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.9
comment:20 Changed 5 years ago by
 Commit changed from 62797e9d80ee0aa3ef22cf87f7cf65725ef8c916 to 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 5 years ago by
 Milestone changed from sage6.9 to sage7.2
comment:22 Changed 5 years ago by
 Branch public/ticket/14683 deleted
 Commit 2b2d683ec628daf2acebe7cd589731535c2a5758 deleted
 Description modified (diff)
 Milestone changed from sage7.2 to sageduplicate/invalid/wontfix
 Status changed from needs_work to needs_review
comment:23 Changed 5 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 5 years ago by
I am focusing on #20695 and will not fix this bug in the mean time.
comment:25 Changed 5 years ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, then
comment:26 Changed 5 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Initial commit: new datastructure for iet.