Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated code from combinat/ #15466

Closed
nathanncohen mannequin opened this issue Nov 29, 2013 · 31 comments
Closed

Remove deprecated code from combinat/ #15466

nathanncohen mannequin opened this issue Nov 29, 2013 · 31 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Nov 29, 2013

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

Nathann

Depends on #15467

CC: @sagetrac-sage-combinat

Component: combinatorics

Author: Nathann Cohen

Branch/Commit: d5086d0

Reviewer: Andrew Mathas, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/15466

@nathanncohen nathanncohen mannequin added this to the sage-5.13 milestone Nov 29, 2013
@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 29, 2013

Dependencies: #15467

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 29, 2013

Branch: u/ncohen/15466

@nathanncohen nathanncohen mannequin changed the title Remove deprecated code from combinat.py Remove deprecated code from combinat/ Nov 29, 2013
@nathanncohen nathanncohen mannequin added the s: needs review label Nov 29, 2013
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2013

Commit: a8bd310

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2013

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

[a8bd310](https://github.com/sagemath/sagetrac-mirror/commit/a8bd310)trac #15466: Remove deprecated code from combinat/
[5993eac](https://github.com/sagemath/sagetrac-mirror/commit/5993eac)trac #15467: Partitions should check its input "a bit" more carefully

@AndrewMathas
Copy link
Member

Changed branch from u/ncohen/15466 to u/andrew.mathas/ticket/15466

@AndrewMathas
Copy link
Member

comment:4

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:

[6639e75](https://github.com/sagemath/sagetrac-mirror/commit/6639e75)A few more deprecations

@AndrewMathas
Copy link
Member

Reviewer: Andrew Mathas

@AndrewMathas
Copy link
Member

Changed commit from a8bd310 to 6639e75

@AndrewMathas AndrewMathas modified the milestones: sage-5.13, sage-6.0 Dec 5, 2013
@AndrewMathas

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 5, 2013

comment:6

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

@vbraun
Copy link
Member

vbraun commented Dec 17, 2013

comment:9
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}
**********************************************************************

@vbraun vbraun reopened this Dec 17, 2013
@tscrim
Copy link
Collaborator

tscrim commented Jan 21, 2014

comment:10

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.

@tscrim
Copy link
Collaborator

tscrim commented Jan 21, 2014

Changed branch from u/andrew.mathas/ticket/15466 to u/tscrim/ticket/15466

@tscrim
Copy link
Collaborator

tscrim commented Jan 21, 2014

Changed commit from 6639e75 to cd43d7b

@AndrewMathas
Copy link
Member

comment:11

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

@vbraun
Copy link
Member

vbraun commented Feb 21, 2014

comment:13

conflict, please merge in latest beta

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2014

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2014

Changed commit from cd43d7b to 25488e8

@tscrim
Copy link
Collaborator

tscrim commented Feb 22, 2014

comment:15

Done.

@tscrim
Copy link
Collaborator

tscrim commented Feb 22, 2014

Changed reviewer from Andrew Mathas to Andrew Mathas, Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Feb 22, 2014

comment:16

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

@tscrim
Copy link
Collaborator

tscrim commented Feb 22, 2014

comment:17

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

@vbraun
Copy link
Member

vbraun commented Feb 22, 2014

comment:18

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

@vbraun
Copy link
Member

vbraun commented Mar 4, 2014

comment:19

6.2.beta3 is out ;-)

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 5, 2014

comment:20

Done !

Nathann


New commits:

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

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 5, 2014

Changed commit from 25488e8 to d5086d0

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 5, 2014

Changed branch from u/tscrim/ticket/15466 to u/ncohen/15466

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2014

comment:21

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

PS - Thanks Nathann.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Mar 5, 2014

comment:22

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

@vbraun
Copy link
Member

vbraun commented Mar 6, 2014

Changed branch from u/ncohen/15466 to d5086d0

@vbraun vbraun closed this as completed in 4c30132 Mar 6, 2014
tscrim pushed a commit to tscrim/sage that referenced this issue Jun 1, 2023
* develop: (100 commits)
  Updated Sage version to 6.2.beta4
  added documentation, patchlevel bump
  trac sagemath#15894 modif of two warnings
  delete cruft from the guava package
  trac sagemath#15893 review patch
  trac sagemath#15894 another backward change
  trac sagemath#15894 changin back the category names
  trac #15xxx proper names should start by a capital letter
  Fix changelog entry for ell_wp
  trac sagemath#15893: graphs.petersen_family
  Fixes to code from sagemath#9505.
  Fix the banner displayed by sage -h
  trac sagemath#15466: remove dead import
  trac sagemath#15795 line wrap
  trac sagemath#15795 redone with ( ::)
  Fraction to power series: doctests
  Fraction to power series conversion when the fraction coerces into the base ring
  15698: Allow construction of power series from fractions
  Create *unevaluated* integrals when converting sage integrals to sympy
  Use evaluate=False in SympyConverter too
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants