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:

Status badges

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 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 7 years ago by vdelecroix

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

New commits:

530fe85Initial commit: new datastructure for iet.

comment:4 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 7 years ago by chapoton

  • 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:

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

comment:6 Changed 7 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 7 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 7 years ago by vdelecroix

  • 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 git

  • Commit changed from 4d69202504be026bb76ced36c68c96d2f2da9baa to 7fe873450d232ad485a650e3bb5c701456c6a52f

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

7fe8734trac #14683 pour raise statements into py3 syntax

comment:10 Changed 7 years ago by git

  • Commit changed from 7fe873450d232ad485a650e3bb5c701456c6a52f to 9464f5cff7685334f8298e300f46f20c6738dec3

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 7 years ago by git

  • Commit changed from 9464f5cff7685334f8298e300f46f20c6738dec3 to cf153d84fd9539520d1374b6fe2f98bc3ec93f93

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 7 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 7 years ago by git

  • Commit changed from cf153d84fd9539520d1374b6fe2f98bc3ec93f93 to db9430e3dfa371ac37f9842974d3fc3e7d35f3fa

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 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:15 follow-up: Changed 6 years ago by jdemeyer

  • 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 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 6 years ago by jdemeyer

  • Status changed from needs_info to needs_work

comment:18 Changed 6 years ago by git

  • Commit changed from db9430e3dfa371ac37f9842974d3fc3e7d35f3fa to 62797e9d80ee0aa3ef22cf87f7cf65725ef8c916

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 6 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.9

comment:20 Changed 5 years ago by git

  • Commit changed from 62797e9d80ee0aa3ef22cf87f7cf65725ef8c916 to 2b2d683ec628daf2acebe7cd589731535c2a5758

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

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

comment:21 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.9 to sage-7.2

comment:22 Changed 5 years ago by vdelecroix

  • Authors Vincent Delecroix deleted
  • 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 chapoton

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 vdelecroix

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

comment:25 Changed 5 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, then

comment:26 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.