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 by itertools.chain
  • map -- replaced by ... map
  • element -- replaced by itertools.repeat
  • select -- replaced by itertools.ifilter
  • successor -- "almost" replaced by itertools.takewhile

Nathann

Attachments (1)

trac_14355.patch (4.7 KB) - added by ncohen 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by vdelecroix

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

comment:3 Changed 6 years ago by ncohen

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 ncohen

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 ncohen

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:7 Changed 6 years ago by ncohen

  • Type changed from PLEASE CHANGE to defect

comment:8 Changed 6 years ago by ncohen

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 tscrim

  • Authors set to Nathann Cohen
  • 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 jdemeyer

  • Milestone changed from sage-5.9 to sage-5.10

comment:11 Changed 6 years ago by jdemeyer

  • 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 ncohen

Green light from the patchbot in beta2 and something uses it in 5.10.beta0 ?... Honestly -_-

Nathann

comment:13 Changed 6 years ago by ncohen

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 ncohen

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 tscrim

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 ncohen

comment:16 Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

Fixed ! And ready for another review ^^;

Nathann

comment:17 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

Looks good to me after running sync-build.

comment:18 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.