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:

GitHub link to the corresponding issue

Description (last modified by vdelecroix)

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 jdemeyer

Milestone: sage-5.11sage-5.12

comment:2 Changed 9 years ago by vbraun_spam

Milestone: sage-6.1sage-6.2

comment:3 Changed 9 years ago by vdelecroix

Branch: u/vdelecroix/14683-iet_new_datastructure
Cc: Fougeroc added
Commit: 530fe858e831dc986235125a0baa3630bbc58505
Keywords: iet flat surfaces added

New commits:

530fe85Initial commit: new datastructure for iet.

comment:4 Changed 9 years ago by vbraun_spam

Milestone: sage-6.2sage-6.3

comment:5 Changed 9 years ago by chapoton

Branch: u/vdelecroix/14683-iet_new_datastructurepublic/ticket/14683
Commit: 530fe858e831dc986235125a0baa3630bbc585054d69202504be026bb76ced36c68c96d2f2da9baa

rebased. Is this needs review ?


New commits:

4d69202Merge branch 'u/vdelecroix/14683-iet_new_datastructure' of ssh://trac.sagemath.org:22/sage into 14683

comment:6 Changed 9 years ago by vdelecroix

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 chapoton

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 vdelecroix

Status: newneeds_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 git

Commit: 4d69202504be026bb76ced36c68c96d2f2da9baa7fe873450d232ad485a650e3bb5c701456c6a52f

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

7fe8734trac #14683 pour raise statements into py3 syntax

comment:10 Changed 9 years ago by git

Commit: 7fe873450d232ad485a650e3bb5c701456c6a52f9464f5cff7685334f8298e300f46f20c6738dec3

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

c4255b3Merge with 6.3.beta6
9464f5ctrac #14683 cleanup of constructor.py (raise in py3, use of ....:, etc)

comment:11 Changed 9 years ago by git

Commit: 9464f5cff7685334f8298e300f46f20c6738dec3cf153d84fd9539520d1374b6fe2f98bc3ec93f93

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

ae5d4b5trac #14683 cleanup of iet.py
dc8e33ftrac #14683 cleanup of labelled.py
cf153d8trac #14683 cleanup of reduced.py

comment:12 Changed 9 years ago by chapoton

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 git

Commit: cf153d84fd9539520d1374b6fe2f98bc3ec93f93db9430e3dfa371ac37f9842974d3fc3e7d35f3fa

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

880af14trac #14683 cleanup of template.py
db9430etrac #14683 start of pyflakes cleanup

comment:14 Changed 9 years ago by vbraun_spam

Milestone: sage-6.3sage-6.4

comment:15 Changed 8 years ago by jdemeyer

Status: needs_reviewneeds_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 8 years ago by vdelecroix

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 length

and replacing the raise ValueError with an assert?

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 jdemeyer

Status: needs_infoneeds_work

comment:18 Changed 8 years ago by git

Commit: db9430e3dfa371ac37f9842974d3fc3e7d35f3fa62797e9d80ee0aa3ef22cf87f7cf65725ef8c916

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

fc249b6Merge branch 'public/ticket/14683' into 6.8
62797e9trac #14683 rebased and all tests pass

comment:19 Changed 8 years ago by chapoton

Milestone: sage-6.4sage-6.9

comment:20 Changed 7 years ago by git

Commit: 62797e9d80ee0aa3ef22cf87f7cf65725ef8c9162b2d683ec628daf2acebe7cd589731535c2a5758

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

2b2d683Merge branch 'public/ticket/14683' into 7.2.rc0

comment:21 Changed 7 years ago by chapoton

Milestone: sage-6.9sage-7.2

comment:22 Changed 7 years ago by vdelecroix

Authors: Vincent Delecroix
Branch: public/ticket/14683
Commit: 2b2d683ec628daf2acebe7cd589731535c2a5758
Description: modified (diff)
Milestone: sage-7.2sage-duplicate/invalid/wontfix
Status: needs_workneeds_review

comment:23 Changed 7 years ago by chapoton

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 vdelecroix

I am focusing on #20695 and will not fix this bug in the mean time.

comment:25 Changed 7 years ago by chapoton

Reviewers: Frédéric Chapoton
Status: needs_reviewpositive_review

ok, then

comment:26 Changed 7 years ago by vbraun

Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.