Opened 11 years ago

Closed 8 years ago

#8386 closed defect (fixed)

move iet from sage.combinat to sage.dynamics

Reported by: vdelecroix Owned by: vdelecroix
Priority: major Milestone: sage-5.12
Component: combinatorics Keywords: iet, combinatorics
Cc: sage-combinat, tmonteil Merged in: sage-5.12.beta0
Authors: Frédéric Chapoton Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12643, #13677, #14669 Stopgaps:

Status badges

Description (last modified by chapoton)

The library for iet is moved from sage.combinat.iet to sage.dynamics.interval_exchanges which seems more natural.

Apply

Follow-up: #14683

Attachments (8)

trac_8386_iet_alphabet_bug.patch (142.0 KB) - added by vdelecroix 11 years ago.
trac_8386-enhanced_iet.patch (668.1 KB) - added by vdelecroix 9 years ago.
apply only this one
trac_8386-enhanced_iet.v2.patch (670.0 KB) - added by chapoton 8 years ago.
trac_8386-enhanced_iet.v3.patch (520.6 KB) - added by chapoton 8 years ago.
trac_8386_just_moving-fc.patch (673.7 KB) - added by chapoton 8 years ago.
trac_8386_really_just_moving-fc.patch (2.1 KB) - added by chapoton 8 years ago.
trac_8386_big_clean_fc.patch (169.7 KB) - added by chapoton 8 years ago.
trac_8386_assert_removal.patch (19.7 KB) - added by chapoton 8 years ago.

Change History (54)

Changed 11 years ago by vdelecroix

comment:1 Changed 11 years ago by vdelecroix

  • Component changed from algebra to combinatorics
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by tmonteil

  • Cc tmonteil added

comment:3 Changed 10 years ago by ncohen

  • Status changed from needs_review to needs_work

This patch really needs to be rebased (14 hunk failures) :-)

comment:4 Changed 9 years ago by vdelecroix

  • Dependencies set to #12643
  • Description modified (diff)
  • Keywords combinatorics added; combinatoric removed
  • Summary changed from iet alphabet bug and rebased datatype to enhanced version of iet

comment:5 Changed 9 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:6 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to needs_work

A bunch of doctests are failing with this patch (see patchbot logs)

comment:7 Changed 9 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:8 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to doctest failure

This seems to work with 4.8 but two doctests consistently fail with recent 5.0 betas:

**********************************************************************
File "/storage/masiao/sage-5.0.beta12-patchbot/devel/sage-8386/sage/dynamics/flat_surfaces/abelian_strata.py", line 1348:
    sage: c1 != c2_hyp
Exception raised:
    Traceback (most recent call last):
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_38[13]>", line 1, in <module>
        c1 != c2_hyp###line 1348:
    sage: c1 != c2_hyp
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/lib/python/site-packages/sage/dynamics/flat_surfaces/abelian_strata.py", line 1364, in __cmp__
        return type.__cmp__(type(self),type(other))
    AttributeError: type object 'type' has no attribute '__cmp__'
**********************************************************************
File "/storage/masiao/sage-5.0.beta12-patchbot/devel/sage-8386/sage/dynamics/flat_surfaces/abelian_strata.py", line 1350:
    sage: c2_hyp != c2_odd
Exception raised:
    Traceback (most recent call last):
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_38[14]>", line 1, in <module>
        c2_hyp != c2_odd###line 1350:
    sage: c2_hyp != c2_odd
      File "/storage/masiao/sage-5.0.beta12-patchbot/local/lib/python/site-packages/sage/dynamics/flat_surfaces/abelian_strata.py", line 1364, in __cmp__
        return type.__cmp__(type(self),type(other))
    AttributeError: type object 'type' has no attribute '__cmp__'
**********************************************************************

Changed 9 years ago by vdelecroix

apply only this one

comment:9 Changed 9 years ago by vdelecroix

  • Status changed from needs_work to needs_review

I modify one line in the cmp method of AbelianStratumComponent? in order to make it works.

I suspect an upgrade version of python between sage-4.8 and sage-5.0 as I used the method cmp of 'type' which no longer exists.

comment:10 Changed 9 years ago by jdemeyer

Please fill in your real name as Author.

comment:11 Changed 9 years ago by vdelecroix

  • Authors changed from vdelecroix to Vincent Delecroix

comment:12 Changed 9 years ago by chapoton

  • Description modified (diff)

Apply only trac_8386-enhanced_iet.v2.patch

comment:13 Changed 9 years ago by chapoton

The bot is not happy: one doctest is missing in dynamics/interval_exchanges/constructors.py

I also complains about the startup, but I do no know what to do about that..

comment:14 Changed 9 years ago by chapoton

I have added the missing doc.

What about the startup problem ?

comment:15 Changed 9 years ago by chapoton

Apply only trac_8386-enhanced_iet.v2.patch

comment:16 Changed 9 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Work issues changed from doctest failure to startup problem

comment:17 Changed 8 years ago by chapoton

Apply only trac_8386-enhanced_iet.v2.patch

Here is a tentative of lazy_import. Let us see what the bot think of that one.

Changed 8 years ago by chapoton

comment:18 Changed 8 years ago by chapoton

Apply only trac_8386-enhanced_iet.v2.patch

another try, even more lazy !

comment:19 Changed 8 years ago by chapoton

  • Dependencies changed from #12643 to #12643, #13677

Changed 8 years ago by chapoton

comment:20 Changed 8 years ago by chapoton

  • Description modified (diff)

I have rebased on 5.5rc0.

I hope I have not made a mistake in doing that

Apply trac_8386-enhanced_iet.v3.patch

comment:21 Changed 8 years ago by chapoton

  • Description modified (diff)
  • Work issues changed from startup problem to save the new datatype ?

Let me try to do something for this ticket.

Here is a new patch, that only moves the files from "combinat/iet" to a new folder "dynamics".

This has been done by starting with the code in 5.10.beta3. So there is something lost, which is the refactoring of datatype.

But it pass all tests.

comment:22 Changed 8 years ago by chapoton

for the bot:

apply trac_8386_just_moving-fc.patch

comment:23 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues changed from save the new datatype ? to save the new datatype ?; hg mv

If you move files, use hg mv for that instead of hg remove the old and hg add the new file.

comment:24 Changed 8 years ago by vdelecroix

I am happy for this ticket if it simply moves iet to sage.dynamics (and keep the refactoring for another ticket) ! But as jdemeyer mentioned it should be done with hg mv (which is a one line patch) instead of hg remove/hg add (which is a 2x(length of the file) patch long).

vincent

comment:25 follow-ups: Changed 8 years ago by chapoton

Well, I did not now about hg mv.

I have done a lot of work to make the re-location function, including some work on the toc-trees that was rather painful.

And I has also taken the opportunity to

  • put all the raise syntax into python3 form
  • correct orthographic errors
  • make the code much closer to pep8 level
  • removed unused import statements
  • make the desired separation into interval_exchanges and flat_surfaces

I spent a few hours today on that, and I have already spent a lot of time on this ticket some months ago, but nobody did react at that time.

So, given that all tests pass now, it would be great if my work is not lost again.

comment:26 in reply to: ↑ 25 Changed 8 years ago by vdelecroix

Replying to chapoton:

Well, I did not now about hg mv.

I have done a lot of work to make the re-location function, including some work on the toc-trees that was rather painful.

And I has also taken the opportunity to

  • put all the raise syntax into python3 form
  • correct orthographic errors
  • make the code much closer to pep8 level
  • removed unused import statements
  • make the desired separation into interval_exchanges and flat_surfaces

I spent a few hours today on that, and I have already spent a lot of time on this ticket some months ago, but nobody did react at that time.

So, given that all tests pass now, it would be great if my work is not lost again.

I will have a closer look right now at your patch. Nevertheless patchbot complains (blue color instead of green) because the doctest framework now wants "....:" instead of "..." in multiline doctests.

Changed 8 years ago by chapoton

comment:27 Changed 8 years ago by chapoton

for the bot: apply trac_8386_just_moving-fc.patch

here is a patch that should correct the ....: point

let us see what the patchbot says

comment:28 in reply to: ↑ 25 Changed 8 years ago by nthiery

Replying to chapoton:

Well, I did not now about hg mv. ... So, given that all tests pass now, it would be great if my work is not lost again.

You can quickly recreate your patch using hg mv as follow:

  • With your patch applied, backup the files that have been moved/modified
  • Unapply the patch
  • Replay the moving the files using hg mv
  • Reinstate the backed-up files on top of it.
  • hg qnew a new patch with those, and discard the other one.

You might want to actually do two patches, one with just the moving, and the other with the changes.

Btw: thanks Frédéric for all the hard work you are doing lately (cleaning, reviewing and more ...)!

Cheers,

Nicolas

comment:29 Changed 8 years ago by chapoton

Thanks Nicolas for this good advice. I am doing that, and I will upload 2 patches

comment:30 Changed 8 years ago by jdemeyer

There is even an option

hg mv --after

which allows one to mark the move after it has happened. But it doesn't work well with queues, as a patch which has been qrefreshed is considered committed. So you need a non-committed version of the patch: apply the patch "by hand" using patch -p1 <x.patch, then hg mv --after, then hg commit should work (but I haven't really tried).

Last edited 8 years ago by jdemeyer (previous) (diff)

Changed 8 years ago by chapoton

comment:31 follow-up: Changed 8 years ago by chapoton

  • Description modified (diff)

ok, here it is. It seems that my doc framework is now completely broken (I have by mistake removed the output directory and now docbuild complains a lot about intersphinx and missing files), so I do not know if the doc is ok.

for the bot:

apply trac_8386_really_just_moving-fc.patch trac_8386_big_clean_fc.patch

comment:32 in reply to: ↑ 31 Changed 8 years ago by vdelecroix

Replying to chapoton:

ok, here it is. It seems that my doc framework is now completely broken (I have by mistake removed the output directory and now docbuild complains a lot about intersphinx and missing files), so I do not know if the doc is ok.

It also happens for me. After removing the directory doc/output it works fine again (but you need to rebuild everything with "sage -docbuild all html"). If there is a less extremal solution then I would be happy to know.

apply trac_8386_really_just_moving-fc.patch trac_8386_big_clean_fc.patch

comment:33 Changed 8 years ago by chapoton

  • Status changed from needs_work to needs_review
  • Work issues changed from save the new datatype ?; hg mv to save the new datatype ?

comment:34 Changed 8 years ago by vdelecroix

  • Authors changed from Vincent Delecroix to Frédéric Chapoton
  • Description modified (diff)
  • Reviewers changed from Frédéric Chapoton to Vincent Delecroix
  • Status changed from needs_review to positive_review
  • Summary changed from enhanced version of iet to move iet from sage.combinat to sage.dynamics
  • Work issues save the new datatype ? deleted

Thanks Frederic.

I put you as an author and me as a reviewer as it is more like that now. I postponed the modification of the implementation to #14683.

comment:35 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:36 Changed 8 years ago by jdemeyer

  • Dependencies changed from #12643, #13677 to #12643, #13677, #14669
  • Description modified (diff)
  • Status changed from positive_review to needs_work

Needs to be rebased to #14669.

Changed 8 years ago by chapoton

comment:37 Changed 8 years ago by chapoton

  • Status changed from needs_work to needs_review

rebased on top of #14669

comment:38 Changed 8 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Great !

comment:39 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Never use assert to check user input, use raise TypeError() or other exceptions for that. An AssertionError appearing in a public function is by definition a bug.

Example:

sage: QuadraticStratum("foo")
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-2-ba2d4d1c0bfc> in <module>()
----> 1 QuadraticStratum("foo")

/mazur/release/merger/sage-5.10/local/lib/python2.7/site-packages/sage/misc/lazy_import.so in sage.misc.lazy_import.LazyImport.__call__ (sage/misc/lazy_import.c:2475)()

/mazur/release/merger/sage-5.10/local/lib/python2.7/site-packages/sage/misc/lazy_import.so in sage.misc.lazy_import.LazyImport.__call__ (sage/misc/lazy_import.c:2475)()

/mazur/release/merger/sage-5.10/local/lib/python2.7/site-packages/sage/dynamics/flat_surfaces/quadratic_strata.pyc in __init__(self, *l)
     30         else:
     31             for i in l:
---> 32                 assert(isinstance(i, (int, Integer)))
     33             self._zeroes = sorted(list(l), reverse=True)
     34 

AssertionError: 

Changed 8 years ago by chapoton

comment:40 Changed 8 years ago by chapoton

  • Description modified (diff)
  • Status changed from needs_work to needs_review

here is patch that

  • removes all "assert" statements, using some "raise xxxError()" instead
  • makes an effort to doctest more of the "raise xxxError()"

There remains to make sure all "raise" are doctested, but I would prefer to keep that for another ticket, given the age of this one.

Needs review

comment:41 Changed 8 years ago by jdemeyer

I haven't checked in detail (Vincent: can you do that please), but it looks good on first sight. Excellent work!

comment:42 Changed 8 years ago by nthiery

Sounds good to me too. Thanks Frédéric!

Btw: I noticed a repr; but that's probably for another ticket.

comment:43 Changed 8 years ago by tscrim

Vincent, are you able to do the re-review in the next few days? #14772 depends on this ticket because there are two instances in iet which use Permutation_class.

comment:44 Changed 8 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Good to go! Thanks Frederic.

comment:45 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:46 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.