move iet from sage.combinat to sage.dynamics

[vdelecroix]

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

Apply

• trac_8386_really_just_moving-fc.patch​
• trac_8386_big_clean_fc.patch​
• trac_8386_assert_removal.patch​

Follow-up: #14683

[ncohen]

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

[davidloeffler]

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

[davidloeffler]

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__'
**********************************************************************

[vdelecroix]

apply only this one

[vdelecroix]

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.

[jdemeyer]

Please fill in your real name as Author.

[chapoton]

Apply only trac_8386-enhanced_iet.v2.patch

[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..

[chapoton]

I have added the missing doc.

What about the startup problem ?

[chapoton]

Apply only trac_8386-enhanced_iet.v2.patch

[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.

[chapoton]

Apply only trac_8386-enhanced_iet.v2.patch

another try, even more lazy !

[chapoton]

I have rebased on 5.5rc0.

I hope I have not made a mistake in doing that

Apply trac_8386-enhanced_iet.v3.patch

[chapoton]

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.

[chapoton]

for the bot:

apply trac_8386_just_moving-fc.patch

[jdemeyer]

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

[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

[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.

[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.

[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

[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

[chapoton]

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

[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).

[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.

for the bot:

apply trac_8386_really_just_moving-fc.patch trac_8386_big_clean_fc.patch

[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

[vdelecroix]

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.

[jdemeyer]

Needs to be rebased to #14669.

[chapoton]

rebased on top of #14669

[vdelecroix]

Great !

[jdemeyer]

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: 

[chapoton]

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

[jdemeyer]

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

[nthiery]

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

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

[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.

[vdelecroix]

Good to go! Thanks Frederic.