Move RecursivelyEnumeratedSet_forest code to sage/sets/recursively_enumerated_set.pyx, remove deprecated classes SearchForest, TransitiveIdeal, TransitiveIdealGraded
Closed this issue · 40 comments
Move the code of SearchForest from sage/combinat/backtrack.py to sage/sets/recursively_enumerated_set.pyx into a class named RecursivelyEnumeratedSet_forest.
This is a follow up of #6637.
Component: combinatorics
Author: Matthias Koeppe
Branch: 908acba
Reviewer: Vincent Delecroix
Issue created by migration from https://trac.sagemath.org/ticket/16351
I just added some old doc from Nathann and myself which is worth being integrated when cleaning this up.
Last 10 new commits:
7a33037 | Work in progress on the DOC |
168ceae | Added architecture picture for map_reduce |
366fc17 | More Doc |
6c12752 | Tested timeout option |
493bb52 | Done the doc of Map/Reduce |
2534f12 | Moved map/reduce to sage/parallel |
e5b7477 | Merge 6.10.rc1 + fixed conflict |
82fd1e4 | Fixed links according to deprecation/rebase |
68b6530 | Removed TODO in doc |
2effaf7 | Revert "Removed TODO in doc" |
I would like to squeeze out some more speed of the SearchForest code, so I'm interested in this. I made #13580 a dependency since that is likely a lot closer than this is to getting into Sage. If you disagree, then we can reverse the order of the dependencies and I'd do the review once this is ready.
The actual branch contains many commits from the #13580, so I can't see what is the goal of the branch put here and how it is related to this ticket.
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
f984165 | Fixed the doc |
a5f4d67 | Improved doc for map-reduce |
569de0d | Merge branch 'develop' into t/13580/map_reduce |
b07dfe2 | Doc of the two implementationsof ActiveTaskCounter |
beebcbc | #13580: Added comment on timing in the doc |
58eca2e | #13580: Removed comment which is now in the doc |
1badd8a | #13580: Renamed ActiveTaskCounter(Posix) |
46cbab9 | 13580: Fixed doctests to pass on Darwin |
134c1fa | 13580: doc rereading |
1bf8c97 | Merge branch 'u/hivert/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx' of git://trac.sagemath.org/sage into t/13580/map_reduce |
Replying to @seblabbe:
The actual branch contains many commits from the #13580, so I can't see what is the goal of the branch put here and how it is related to this ticket.
I just rebased this ticket on top of #13580 the code contains only an explanation of what SearchForest does with a nice picture from Nathann, Nicolas and nyself. It should be integrated during the move and the documentation cleanup.
Something else that we should do while-we-are-at-it, we should consider using collections.deque since it supports fast popping from both ends (list can become really slow when popping at the front, but deque is still twice as slow popping at the front).
Now that #13580 is in, the current branch contains just a small
self-contained documentation improvement. It seems worth and easy to
integrate soon, but somehow unrelated to this ticket, right?
Shall we create a separate ticket for that documentation chunk?
And refocus this one on moving the SearchForest code to
sage/sets/recursively_enumerated_set.pyx (more than by just a single
alias)?
Replying to @nthiery:
Now that #13580 is in, the current branch contains just a small
self-contained documentation improvement. It seems worth and easy to
integrate soon, but somehow unrelated to this ticket, right?Shall we create a separate ticket for that documentation chunk?
Please yes.
And refocus this one on moving the SearchForest code to
sage/sets/recursively_enumerated_set.pyx (more than by just a single
alias)?
Yes.
Replying to @seblabbe:
Replying to @nthiery:
Now that #13580 is in, the current branch contains just a small
self-contained documentation improvement. It seems worth and easy to
integrate soon, but somehow unrelated to this ticket, right?Shall we create a separate ticket for that documentation chunk?
Please yes.
This is now #29882.
Changed branch from u/hivert/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx to none
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
364d7b3 | Move SearchForest code to sage/sets/recursively_enumerated_set.pyx |
68f7bd5 | sage.combinat.integer_vectors_mod_permgroup: Update to use RecursivelyEnumeratedSet_forest |
1764ca4 | Update remaining uses of SearchForest to use RecursivelyEnumeratedSet_forest |
Let's see what the patchbot has to say.
I have made not made any attempt to cythonize anything; this is really just pure Python code moved into the pyx file.
Author: Matthias Koeppe
Also, I added a lazy_import for SearchForest but it seems that SearchForest was already deprecated -- perhaps now it can be removed completely?
Branch pushed to git repo; I updated commit sha1. New commits:
1ac0a09 | sage.combinat.backtrack: Remove deprecated class SearchForest |
Also TransitiveIdeal and TransitiveIdealGraded were deprecated in #6637.
Branch pushed to git repo; I updated commit sha1. New commits:
908acba | sage.combinat.backtrack: Remove deprecated classes TransitiveIdeal and TransitiveIdealGraded |
It seems to me that with your changes SearchForest does not exist anymore. If so, you must add a lazy import with deprecation warning in backtrack.py. The SearchForest was documented as a deprecated class but was not making any deprecation warning when used.
That's right, these deprecated classes are deleted.
The deprecation happened in #6637 and announced on the ticket, I guess before deprecation warnings were invented?
Reviewer: Vincent Delecroix
This is somehow annoying. Let us hope that nobody uses it in her code.
Thanks!
Changed branch from u/mkoeppe/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx to 908acba
Thanks for finishing this task.I feel ashame I never finished it myself.
As a further todo, I think it will be nice to have
-class RecursivelyEnumeratedSet_forest(Parent):
+class RecursivelyEnumeratedSet_forest(RecursivelyEnumeratedSet_generic):There are few methods like to_digraph from the generic class that I like to use but are currently not available for the forest type.