Opened 10 years ago
Closed 7 years ago
#14683 closed enhancement (fixed)
New datastructure for iet
Reported by: | vdelecroix | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/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: | sage-5.11 → sage-5.12 |
---|
comment:2 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:3 Changed 9 years ago by
Branch: | → u/vdelecroix/14683-iet_new_datastructure |
---|---|
Cc: | Fougeroc added |
Commit: | → 530fe858e831dc986235125a0baa3630bbc58505 |
Keywords: | iet flat surfaces added |
comment:4 Changed 9 years ago by
Milestone: | sage-6.2 → sage-6.3 |
---|
comment:5 Changed 9 years ago by
Branch: | u/vdelecroix/14683-iet_new_datastructure → public/ticket/14683 |
---|---|
Commit: | 530fe858e831dc986235125a0baa3630bbc58505 → 4d69202504be026bb76ced36c68c96d2f2da9baa |
rebased. Is this needs review ?
New commits:
4d69202 | Merge branch 'u/vdelecroix/14683-iet_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/14683-iet_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 sage-X.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: | sage-6.3 → sage-6.4 |
---|
comment:15 follow-up: 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: | sage-6.4 → sage-6.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: | sage-6.9 → sage-7.2 |
---|
comment:22 Changed 7 years ago by
Authors: | Vincent Delecroix |
---|---|
Branch: | public/ticket/14683 |
Commit: | 2b2d683ec628daf2acebe7cd589731535c2a5758 |
Description: | modified (diff) |
Milestone: | sage-7.2 → sage-duplicate/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.