#16351 closed enhancement (fixed)
Move RecursivelyEnumeratedSet_forest code to sage/sets/recursively_enumerated_set.pyx, remove deprecated classes SearchForest, TransitiveIdeal, TransitiveIdealGraded
Reported by:  slabbe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  combinatorics  Keywords:  
Cc:  tscrim, slelievre  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  908acba (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
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.
Change History (32)
comment:1 Changed 8 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:2 Changed 6 years ago by
 Branch set to u/hivert/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx
comment:3 Changed 6 years ago by
 Commit set to 2effaf76cddfe91175187d8ba47d8bf090e0dec0
comment:4 Changed 6 years ago by
 Dependencies set to #13580
 Milestone changed from sage6.4 to sage7.0
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.
comment:5 followup: ↓ 7 Changed 6 years ago by
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.
comment:6 Changed 6 years ago by
 Commit changed from 2effaf76cddfe91175187d8ba47d8bf090e0dec0 to 1bf8c97ef9e6479ede07b339b11d413819508307
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
f984165  Fixed the doc

a5f4d67  Improved doc for mapreduce

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

comment:7 in reply to: ↑ 5 Changed 6 years ago by
Replying to slabbe:
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.
comment:8 Changed 6 years ago by
Something else that we should do whileweareatit, 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).
comment:9 followup: ↓ 10 Changed 5 years ago by
Now that #13580 is in, the current branch contains just a small selfcontained 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)?
comment:10 in reply to: ↑ 9 ; followup: ↓ 12 Changed 5 years ago by
Replying to nthiery:
Now that #13580 is in, the current branch contains just a small selfcontained 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.
comment:11 Changed 2 years ago by
 Cc tscrim added
 Milestone changed from sage7.0 to sage9.2
comment:12 in reply to: ↑ 10 Changed 2 years ago by
Replying to slabbe:
Replying to nthiery:
Now that #13580 is in, the current branch contains just a small selfcontained 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.
comment:13 Changed 2 years ago by
 Branch u/hivert/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx deleted
 Commit 1bf8c97ef9e6479ede07b339b11d413819508307 deleted
 Dependencies #13580 deleted
comment:14 Changed 2 years ago by
 Branch set to u/mkoeppe/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx
comment:15 Changed 2 years ago by
 Commit set to 143604d960c5b738d28f465c600aee5b2635e06d
comment:16 Changed 2 years ago by
 Commit changed from 143604d960c5b738d28f465c600aee5b2635e06d to 1764ca414390717102378411166d3fd6da457d85
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

comment:17 Changed 2 years ago by
 Status changed from new to needs_review
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.
comment:18 Changed 2 years ago by
Also, I added a lazy_import
for SearchForest
but it seems that SearchForest
was already deprecated  perhaps now it can be removed completely?
comment:19 Changed 2 years ago by
(deprecation in #6637, 6 years ago)
comment:20 Changed 2 years ago by
 Commit changed from 1764ca414390717102378411166d3fd6da457d85 to 1ac0a095626cd70a1b5fabb59474ba3f37f47b12
Branch pushed to git repo; I updated commit sha1. New commits:
1ac0a09  sage.combinat.backtrack: Remove deprecated class SearchForest

comment:21 Changed 2 years ago by
Also TransitiveIdeal
and TransitiveIdealGraded
were deprecated in #6637.
comment:22 Changed 2 years ago by
 Commit changed from 1ac0a095626cd70a1b5fabb59474ba3f37f47b12 to 908acba415758383bbc6092008276b9081ab782d
Branch pushed to git repo; I updated commit sha1. New commits:
908acba  sage.combinat.backtrack: Remove deprecated classes TransitiveIdeal and TransitiveIdealGraded

comment:23 Changed 2 years ago by
 Summary changed from Move SearchForest code to sage/sets/recursively_enumerated_set.pyx to Move RecursivelyEnumeratedSet_forest code to sage/sets/recursively_enumerated_set.pyx, removed deprecated classes SearchForest, TransitiveIdeal, TransitiveIdealGraded
comment:24 Changed 2 years ago by
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.
comment:25 Changed 2 years ago by
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?
comment:26 Changed 2 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
This is somehow annoying. Let us hope that nobody uses it in her code.
comment:27 Changed 2 years ago by
Thanks!
comment:28 Changed 2 years ago by
 Cc slelievre added
 Summary changed from Move RecursivelyEnumeratedSet_forest code to sage/sets/recursively_enumerated_set.pyx, removed deprecated classes SearchForest, TransitiveIdeal, TransitiveIdealGraded to Move RecursivelyEnumeratedSet_forest code to sage/sets/recursively_enumerated_set.pyx, remove deprecated classes SearchForest, TransitiveIdeal, TransitiveIdealGraded
comment:29 Changed 2 years ago by
comment:30 Changed 2 years ago by
 Branch changed from u/mkoeppe/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx to 908acba415758383bbc6092008276b9081ab782d
 Resolution set to fixed
 Status changed from positive_review to closed
comment:31 Changed 23 months ago by
 Commit 908acba415758383bbc6092008276b9081ab782d deleted
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.
comment:32 Changed 22 months ago by
A followup ticket is here: #30238
I just added some old doc from Nathann and myself which is worth being integrated when cleaning this up.
Last 10 new commits:
Work in progress on the DOC
Added architecture picture for map_reduce
More Doc
Tested timeout option
Done the doc of Map/Reduce
Moved map/reduce to sage/parallel
Merge 6.10.rc1 + fixed conflict
Fixed links according to deprecation/rebase
Removed TODO in doc
Revert "Removed TODO in doc"