Opened 11 years ago
Closed 7 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: |
Description (last modified by )
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
Attachments (8)
Change History (54)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Component changed from algebra to combinatorics
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Cc tmonteil added
comment:3 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 9 years ago by
- 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
- Status changed from needs_work to needs_review
comment:6 Changed 9 years ago by
- 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
- Status changed from needs_work to needs_review
comment:8 Changed 9 years ago by
- 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__' **********************************************************************
comment:9 Changed 9 years ago by
- 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 8 years ago by
Please fill in your real name as Author.
comment:11 Changed 8 years ago by
comment:12 Changed 8 years ago by
- Description modified (diff)
Apply only trac_8386-enhanced_iet.v2.patch
comment:13 Changed 8 years ago by
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 8 years ago by
I have added the missing doc.
What about the startup problem ?
comment:15 Changed 8 years ago by
Apply only trac_8386-enhanced_iet.v2.patch
comment:16 Changed 8 years ago by
- Reviewers set to Frédéric Chapoton
- Work issues changed from doctest failure to startup problem
comment:17 Changed 8 years ago by
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
comment:18 Changed 8 years ago by
Apply only trac_8386-enhanced_iet.v2.patch
another try, even more lazy !
comment:19 Changed 8 years ago by
- Dependencies changed from #12643 to #12643, #13677
Changed 8 years ago by
comment:20 Changed 8 years ago by
- 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
- 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
for the bot:
apply trac_8386_just_moving-fc.patch
comment:23 Changed 8 years ago by
- 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
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: ↓ 26 ↓ 28 Changed 8 years ago by
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
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
comment:27 Changed 8 years ago by
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
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
Thanks Nicolas for this good advice. I am doing that, and I will upload 2 patches
comment:30 Changed 8 years ago by
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 qrefresh
ed 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).
Changed 8 years ago by
comment:31 follow-up: ↓ 32 Changed 8 years ago by
- 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
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
- 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
- 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
- Milestone changed from sage-5.10 to sage-5.11
comment:36 Changed 8 years ago by
- 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
comment:37 Changed 8 years ago by
- Status changed from needs_work to needs_review
rebased on top of #14669
comment:39 Changed 8 years ago by
- 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
comment:40 Changed 8 years ago by
- 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
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
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
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
- Status changed from needs_review to positive_review
Good to go! Thanks Frederic.
comment:45 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:46 Changed 7 years ago by
- Merged in set to sage-5.12.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
This patch really needs to be rebased (14 hunk failures)
:-)