Opened 5 years ago

Closed 5 years ago

#15466 closed defect (fixed)

Remove deprecated code from combinat/

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.2
Component: combinatorics Keywords:
Cc: sage-combinat Merged in:
Authors: Nathann Cohen Reviewers: Andrew Mathas, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: d5086d0 (Commits) Commit: d5086d01765e15f320a9399faf5ce521ddf12eaf
Dependencies: #15467 Stopgaps:

Description (last modified by andrew.mathas)

Removes deprecated code from #12930 #13821 #6136 (4 years old) #13072 #9265 together with #8429 #12469 #6519.

Nathann

Change History (23)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/15466
  • Cc sage-combinat added
  • Dependencies set to #15467
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Remove deprecated code from combinat.py to Remove deprecated code from combinat/

comment:2 Changed 5 years ago by git

  • Commit set to a8bd31096c7f93c1c1a51ac03b634d85a07a0dd7

Branch pushed to git repo; I updated commit sha1. New commits:

a8bd310trac #15466: Remove deprecated code from combinat/
5993eactrac #15467: Partitions should check its input "a bit" more carefully

comment:3 Changed 5 years ago by andrew.mathas

  • Branch changed from u/ncohen/15466 to u/andrew.mathas/ticket/15466
  • Created changed from 11/29/13 14:11:00 to 11/29/13 14:11:00
  • Modified changed from 11/29/13 19:24:58 to 11/29/13 19:24:58

comment:4 follow-up: Changed 5 years ago by andrew.mathas

  • Commit changed from a8bd31096c7f93c1c1a51ac03b634d85a07a0dd7 to 6639e7543dc6399280c5e42babca5eff18584da9
  • Milestone changed from sage-5.13 to sage-6.0
  • Reviewers set to Andrew Mathas

Hi Nathann, thanks for starting this (or should that be thaaaaaaaaaaaanks!:).

I found a few more depreciated [sic] functions to remove in the words subdirectory and I moved a few import statements of deprecation so that they are now immediately above the function call. This will help ensure that there are no stray deprecation import statements left when the corresponding soon-to-be-depreciated code is removed. Finally, I killed off some documentation for some functions that are removed by this patch.

If you are happy with my changes let's make this a positive review. I've bumped the version number to 6 since we're using git.

[ps I have checked all of the doctsts in sage/combinat. I do get a doctext failure in root_system/coxeter_group.py but I get the same failure in master, so I think it has nothing to do with this ticket.]


New commits:

6639e75A few more deprecations
Last edited 5 years ago by andrew.mathas (previous) (diff)

comment:5 Changed 5 years ago by andrew.mathas

  • Description modified (diff)

comment:6 in reply to: ↑ 4 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

Yooooooooooo !!

Hi Nathann, thanks for starting this (or should that be thaaaaaaaaaaaanks!:).

Well I'm glad to receive your help with this ! :-D

I found a few more depreciated [sic] functions to remove in the words subdirectory and I moved a few import statements of deprecation so that they are now immediately above the function call. This will help ensure that there are no stray deprecation import statements left when the corresponding soon-to-be-depreciated code is removed.

GoodGoodGood? !

Finally, I killed off some documentation for some functions that are removed by this patch.

Argggg ! Nice job spotting that ;-)

By the way : these days I use the --warn-links flag when building the doc. It tells you whenever some links are made toward non-existent functions. I guess it will be useful to spot things like that in the future, when this --warn-links will be a default, though right now there are too many broken lins in Sage to make it the default :-)

If you are happy with my changes let's make this a positive review. I've bumped the version number to 6 since we're using git.

The tests passed while I was reading the patch. Good to go, thanks :-)

[ps I have checked all of the doctsts in sage/combinat. I do get a doctext failure in root_system/coxeter_group.py but I get the same failure in master, so I think it has nothing to do with this ticket.]

Hmmmmm.. I don't get this error. Though at some point Volker told me that I should run "make" again in Sage's directory to update the spkg which may have changed since. Do give it a try when you don't need to use Sage for a while (it will recompile a lot of things). I used to have a couple of broken doctests, and it solved it !

Thank you again, and positive review to this patch ! ;-)

Nathann

comment:7 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:8 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:9 Changed 5 years ago by vbraun

  • Resolution fixed deleted
  • Status changed from closed to new
sage -t src/sage/misc/cachefunc.pyx
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 534, in sage.misc.cachefunc.CachedFunction.__init__
Failed example:
    g.cache
Expected:
    {((5, None, 'default'), ()): 7}
Got:
    {((5, 'default'), ()): 7}
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 722, in sage.misc.cachefunc.CachedFunction.__call__
Failed example:
    g.get_cache()
Expected:
    {((5, None, 'default'), ()): 7}
Got:
    {((5, 'default'), ()): 7}
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 761, in sage.misc.cachefunc.CachedFunction.get_cache
Failed example:
    g.get_cache()
Expected:
    {((5, None, 'default'), ()): 7}
Got:
    {((5, 'default'), ()): 7}
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 802, in sage.misc.cachefunc.CachedFunction.set_cache
Failed example:
    g.get_cache()
Expected:
    {((5, None, 'default'), ()): 7}
Got:
    {((5, 'default'), ()): 7}
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 805, in sage.misc.cachefunc.CachedFunction.set_cache
Failed example:
    g.get_cache()
Expected:
    {((5, None, 'default'), ()): 17}
Got:
    {((5, 'default'), ()): 17}
**********************************************************************
File "src/sage/misc/cachefunc.pyx", line 867, in sage.misc.cachefunc.CachedFunction.clear_cache
Failed example:
    g.get_cache()
Expected:
    {((5, None, 'default'), ()): 7}
Got:
    {((5, 'default'), ()): 7}
**********************************************************************

comment:10 Changed 5 years ago by tscrim

  • Branch changed from u/andrew.mathas/ticket/15466 to u/tscrim/ticket/15466
  • Commit changed from 6639e7543dc6399280c5e42babca5eff18584da9 to cd43d7b88cd56b54165a8d5c9b19a0e4ed796bb7
  • Status changed from new to needs_review

Fixed the failing doctest.


New commits:

34aa97aMerge branch 'u/andrew.mathas/ticket/15466' of trac.sagemath.org:sage into u/tscrim/ticket/15466
cd43d7bFixed failing doctests for #15466.

comment:11 Changed 5 years ago by andrew.mathas

  • Status changed from needs_review to positive_review

I didn't understand why this patch should affect cachefunc.pyx but the doc-tests causing the problems all call number_of_partitions. Part of this patch removes depreciated code in partition.py and, in particular, it changes the calling syntax by removing a depreciated argument of k=None. This both explains the errors above and says that the correct fix is simply to change the output of the doct-tests as the patch does. Hence, positive review.

Andrew

comment:12 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:13 Changed 5 years ago by vbraun

conflict, please merge in latest beta

comment:14 Changed 5 years ago by git

  • Commit changed from cd43d7b88cd56b54165a8d5c9b19a0e4ed796bb7 to 25488e8974f3d986b5be719e5aaa61d9910becb8
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

25488e8Merge branch 'u/tscrim/ticket/15466' of trac.sagemath.org:sage into u/tscrim/ticket/15466

comment:15 Changed 5 years ago by tscrim

  • Reviewers changed from Andrew Mathas to Andrew Mathas, Travis Scrimshaw
  • Status changed from needs_review to positive_review

Done.

comment:16 Changed 5 years ago by vbraun

Documentation doesn't build

build/pipestatus "./sage --docbuild --no-pdf-links all html -j  2>&1" "tee -a logs/dochtml.log"
Setting permissions of DOT_SAGE directory so only you can read and write it.
Traceback (most recent call last):
  File "/Users/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 1465, in <module>
    import sage.all
  File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/all.py", line 133, in <module>
    from sage.combinat.all   import *
  File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/all.py", line 1, in <module>
    from combinat import bell_number, catalan_number, euler_number, fibonacci, \
ImportError: cannot import name combinations

comment:17 Changed 5 years ago by tscrim

I'm unable to reproduce this. I rebuilt, ran sync-build, rebuilt, nuked my docbuild, and rebuilt the entire doc.

Last edited 5 years ago by tscrim (previous) (diff)

comment:18 Changed 5 years ago by vbraun

Might be due to #9505, please wait until 6.2.beta3 is out and then merge that in.

comment:19 Changed 5 years ago by vbraun

6.2.beta3 is out ;-)

comment:20 Changed 5 years ago by ncohen

  • Branch changed from u/tscrim/ticket/15466 to u/ncohen/15466
  • Commit changed from 25488e8974f3d986b5be719e5aaa61d9910becb8 to d5086d01765e15f320a9399faf5ce521ddf12eaf

Done !

Nathann


New commits:

b4f424btrac #15466: Rebase on 6.2.beta3
d5086d0trac #15466: remove dead import

comment:21 Changed 5 years ago by tscrim

Hmmm...strange... I didn't see those imports when looking at the branch before. *shrugs*

PS - Thanks Nathann.

Last edited 5 years ago by tscrim (previous) (diff)

comment:22 Changed 5 years ago by ncohen

Yeah, I find it strange too.... And I don't see why they could have been added from another patch in the meantime O_o

Nathann

comment:23 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/15466 to d5086d01765e15f320a9399faf5ce521ddf12eaf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.