Opened 8 years ago
Closed 5 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 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:2 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:3 Changed 7 years ago by
- Branch set to u/vdelecroix/14683-iet_new_datastructure
- Cc Fougeroc added
- Commit set to 530fe858e831dc986235125a0baa3630bbc58505
- Keywords iet flat surfaces added
comment:4 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:5 Changed 7 years ago by
- Branch changed from u/vdelecroix/14683-iet_new_datastructure to public/ticket/14683
- Commit changed from 530fe858e831dc986235125a0baa3630bbc58505 to 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 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/14683-iet_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 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 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 sage-6.3 to sage-6.4
comment:15 follow-up: ↓ 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 sage-6.4 to sage-6.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 sage-6.9 to sage-7.2
comment:22 Changed 5 years ago by
- Branch public/ticket/14683 deleted
- Commit 2b2d683ec628daf2acebe7cd589731535c2a5758 deleted
- Description modified (diff)
- Milestone changed from sage-7.2 to sage-duplicate/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.