Opened 6 years ago
Closed 6 years ago
#14355 closed defect (fixed)
Removes sage.combinat.generator
Reported by: | ncohen | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | combinatorics | Keywords: | |
Cc: | vdelecroix | Merged in: | sage-5.10.beta0 |
Authors: | Nathann Cohen | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Vincent noticed in #10534 that this module was completely useless because of itertools, and it even happens that nothing in Sage uses it.
What about getting rid of it ?
It implements:
concat
-- replaced byitertools.chain
map
-- replaced by ...map
element
-- replaced byitertools.repeat
select
-- replaced byitertools.ifilter
successor
-- "almost" replaced byitertools.takewhile
Nathann
Attachments (1)
Change History (19)
comment:1 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
comment:3 Changed 6 years ago by
Come ooooooooooooon. These things are useless. Nobody knows they exist, and those who do will know about itertools....
comment:4 Changed 6 years ago by
So what do we do, we keep this useless thing for another year and ADD code to it ? -_-
Nathann
comment:5 Changed 6 years ago by
Ok let's solve it with Sage-devel. I'll deprecate this stuff if needed but that's really a waste of time.
Nathann
comment:6 Changed 6 years ago by
comment:7 Changed 6 years ago by
- Type changed from PLEASE CHANGE to defect
comment:8 Changed 6 years ago by
Well, given that the only answers on sage-devel seemed to agree with plain removal, and that after my message on sage-combinat [1] nothing was added, I guess that this ticket is still waiting for a review.
Nathann
[1] https://groups.google.com/d/topic/sage-combinat-devel/xW_zvggLm84/discussion
comment:9 Changed 6 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
While I'm not 100% comfortable with outright removal of something in mid-level sage, I doubt anyone will really notice that this is gone (especially since it is only referenced in one place). So on that note, I'm setting this to positive review.
comment:10 Changed 6 years ago by
- Milestone changed from sage-5.9 to sage-5.10
comment:11 Changed 6 years ago by
- Status changed from positive_review to needs_work
Traceback (most recent call last): File "./spkg-install", line 4, in <module> from sage.all import save File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/all.py", line 96, in <module> from sage.algebras.all import * File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/algebras/all.py", line 20, in <module> from quatalg.all import * File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/algebras/quatalg/__init__.py", line 3, in <module> import all File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/algebras/quatalg/all.py", line 1, in <module> from quaternion_algebra import QuaternionAlgebra File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/algebras/quatalg/quaternion_algebra.py", line 62, in <module> import quaternion_algebra_cython File "quaternion_algebra_cython.pyx", line 227, in init sage.algebras.quatalg.quaternion_algebra_cython (sage/algebras/quatalg/quaternion_algebra_cython.cpp:4439) File "classcall_metaclass.pyx", line 279, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (sage/misc/classcall_metaclass.c:932) File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/matrix/matrix_space.py", line 228, in __classcall__ return super(MatrixSpace, cls).__classcall__(cls, base_ring, nrows, ncols, sparse) File "cachefunc.pyx", line 992, in sage.misc.cachefunc.WeakCachedFunction.__call__ (sage/misc/cachefunc.c:5175) File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/structure/unique_representation.py", line 447, in __classcall__ instance = typecall(cls, *args, **options) File "classcall_metaclass.pyx", line 467, in sage.misc.classcall_metaclass.typecall (sage/misc/classcall_metaclass.c:1294) File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/matrix/matrix_space.py", line 256, in __init__ from sage.categories.all import Modules, Algebras File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/categories/all.py", line 126, in <module> from crystals import Crystals File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/categories/crystals.py", line 17, in <module> from sage.combinat.subset import Subsets File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/combinat/subset.py", line 33, in <module> import sage.combinat.subword as subword File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/combinat/subword.py", line 50, in <module> import sage.combinat.combination as combination File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/combinat/combination.py", line 23, in <module> from integer_vector import IntegerVectors File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/combinat/integer_vector.py", line 35, in <module> import integer_list File "/mazur/release/merger/sage-5.10.beta0/local/lib/python2.7/site-packages/sage/combinat/integer_list.py", line 34, in <module> import generator ImportError: No module named generator
comment:12 Changed 6 years ago by
Green light from the patchbot in beta2 and something uses it in 5.10.beta0 ?... Honestly -_-
Nathann
comment:13 Changed 6 years ago by
I don't get it. This import line *IS* in the beta1 version, but all tests do pass on that file O_o
Doctesting 1 file. sage -t --long integer_list.py [197 tests, 0.2 s] ---------------------------------------------------------------------- All tests passed! ----------------------------------------------------------------------
And... With the patch applied, this works :
sage: import sage.combinat.generator
even though the file does not exist anymore O_o
Nathann
comment:14 Changed 6 years ago by
Riiiiiiiight.... Because even though the module has been removed the .pyc
files still exists, and is not removed by sage -b
. >_<
Nathann
comment:15 Changed 6 years ago by
The only place I could find another reference to it was in integer_list.py
(without patch on 5.9.beta1
):
travis@travis-virtualbox:~/sage-5.9.beta1/devel/sage-main/sage$ grep -R "combinat\.generator" . ./combinat/generator.py: sage: list(sage.combinat.generator.concat([[1,2,3],[4,5,6]])) ./combinat/generator.py: sage: list(sage.combinat.generator.map(f,[4,5,6])) ./combinat/generator.py: sage: list(sage.combinat.generator.element(1)) ./combinat/generator.py: sage: list(sage.combinat.generator.element(1, n=3)) ./combinat/generator.py: sage: list(sage.combinat.generator.select(f,range(7))) ./combinat/generator.py: sage: list(sage.combinat.generator.successor(0,f)) travis@travis-virtualbox:~/sage-5.9.beta1/devel/sage-main/sage/combinat$ grep "import *generator" * integer_list.py:import generator
so it should just be a matter of replacing it there as well.
Changed 6 years ago by
comment:16 Changed 6 years ago by
- Status changed from needs_work to needs_review
Fixed ! And ready for another review ^^;
Nathann
comment:17 Changed 6 years ago by
- Status changed from needs_review to positive_review
Looks good to me after running sync-build
.
comment:18 Changed 6 years ago by
- Merged in set to sage-5.10.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
I love patches that remove code and make Sage works better ! But, the best strategy would be to introduce deprecations and not to remove the functions. The reason is that some people may use the iterators in their own codes and that code should not be broken by a new Sage version.
Vincent