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 expired deprecations in sage/combinat #19513

Closed
zabrocki mannequin opened this issue Nov 3, 2015 · 58 comments
Closed

remove expired deprecations in sage/combinat #19513

zabrocki mannequin opened this issue Nov 3, 2015 · 58 comments

Comments

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Nov 3, 2015

A grep of the word deprecation in the source code of sage/combinat indicates that there are many functions which have expired. All warnings which were placed there with the intention of eventually removing them. We will clean up some of this code and remove deprecations which have been in Sage for at least one year (the focus will be on those that have been merged since two years).

Code deprecated from:

CC: @sagetrac-fwclarke @AndrewAtLarge @anneschilling @darijgr @VivianePons @hivert @fchapoton @sagetrac-chrisjamesberg @saliola @ghseeli @sagetrac-sage-combinat @zabrocki @nthiery @sagetrac-alubovsky @tscrim @simon-king-jena @bsalisbury1 @videlec @staroste @nathanncohen @brettpim

Component: combinatorics

Keywords: deprecate

Author: Mike Zabrocki

Branch/Commit: a97a800

Reviewer: Vincent Delecroix

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

@zabrocki zabrocki mannequin added this to the sage-6.10 milestone Nov 3, 2015
@zabrocki zabrocki mannequin added t: performance labels Nov 3, 2015
@jdemeyer
Copy link

jdemeyer commented Nov 3, 2015

comment:1

-1 to this ticket. There is no reason to just remove all deprecations like that. Those deprecations may be removed, that's true. But does this imply that it's the right thing to do? Not always. I doubt that any one person really understands all the deprecated code in Sage and you obviously should not remove deprecations that you don't understand.

Besides that, this ticket is simply too broad. If you want to remove specific deprecations, just remove those. But not all at once.

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

comment:2

Fair enough. At least sage/combinat merits a bit of cleanup and I do know something about those deprecations.

@zabrocki

This comment has been minimized.

@zabrocki zabrocki mannequin added c: combinatorics and removed t: performance labels Nov 3, 2015
@zabrocki zabrocki mannequin changed the title remove expired deprecations remove expired deprecations in sage/combinat Nov 3, 2015
@jm58660
Copy link
Mannequin

jm58660 mannequin commented Nov 3, 2015

comment:3

meet_matrix() and join_matrix() on posets.py were badly deprecated, see #17356. Anyway, there have been so much time that they can be be deleted. (They are of course available, but on (semi)lattices, not on posets.)

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

comment:4

Replying to @jm58660:

meet_matrix() and join_matrix() on posets.py were badly deprecated, see #17356. Anyway, there have been so much time that they can be be deleted. (They are of course available, but on (semi)lattices, not on posets.)

6.5 was released less than a year ago so those can stay a bit longer.

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

Branch: public/ticket/19513

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

Changed keywords from none to deprecate

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

Commit: 18dd976

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

comment:5

I'll give a full list of relevant tickets soon. I'll also add to the cc list to ensure that everyone is aware that these functions are disappearing. I decided my rule was not to touch anything that was deprecated in sage 6.4 or later. There is enough here for the moment.


Last 10 new commits:

4cf2094remove from_row_and_column_length from skew_partition.py #14101
2ac8494remove inf,sup,standard_form,less from set_partition.py #14140
bf1c7baremove deprecations from permutation.py #14772
1682451remove permutation_nk.py #16472, RobinsonSchenstedKnuth* from rsk.py #15142
e327256remove q_bin in sf/kfpoly.py #14496
c85c5afremove SymmetricFunctionAlgebra from sfa.py #15473
2765365crystalographic in root_system/cartan_type.py #14673 plus import statements from RSK
17e58fbremove HighestWeight* from rigged_configurations/* #13872
cbd98cfremove rigged_configurations/all.py lazy_import statements #15882
18dd976remove OrderedAlphabet* from designs/alphabet.py #8920 and functions from designs/incidence_structures.py #16553

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

Author: Mike Zabrocki

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

comment:6

I will still have to go through the documentation and ensure that removed code does not appear there (this was just a first pass).

I just added a large number of people to the cc list hoping that they will all check that the code that they are responsible for deprecating really should be removed.

split_nk.py and choose_nk.py are in ticket #18674

@zabrocki

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Nov 3, 2015

comment:7

I can vouch for everything except that in designs. Nathann, Vincent, I think those are more in your area.

@jdemeyer
Copy link

jdemeyer commented Nov 3, 2015

comment:8

NOTE: whenever deprecated_callable_import() is used (as opposed to other kinds of deprecation), you should only remove the functionality from the global namespace but not remove it completely.

For example, TransitiveIdeal itself is not deprecated, it's just deprecated within src/sage/combinat/all.py. Especially since the deprecation warning says "This class soon will not be available in that way anymore", nothing indicates that the whole class will be removed.

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 3, 2015

comment:9

nothing indicates that the whole class will be removed.

I can restore it if you (or Nicolas or Florent) think that it should remain, however I was following the instruction:

-- Once the deprecation has been there for enough time: delete
-  ``TransitiveIdeal`` and ``TransitiveIealGraded``.

that was in the file. Note that I did not delete SearchForest for that reason.

@zabrocki

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Nov 3, 2015

comment:10

These classes were deprecated in favor of RecursivelyEnumeratedSet in #6637; so we are okay to remove them. (Just for reference, SearchForest is waiting on #16351.)

@jdemeyer
Copy link

jdemeyer commented Nov 3, 2015

comment:11

Replying to @tscrim:

These classes were deprecated in favor of RecursivelyEnumeratedSet in #6637

No, they were not. From #6637 (emphasis mine):

  1. Deprecate TransitiveIdeal, TransitiveIdealGraded and SearchForest as entry point.

B. TransitiveIdeal and TransitiveIealGraded are used in the code of sage/combinat, sage/categories and sage/groups at least. These should be updated to use RecursivelyEnumeratedSet for speed improvements and also to avoid issues explained in C below. See #16352.

Those classes should have been deprecated in #16352, but they weren't.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

Changed commit from 18dd976 to 946267b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2015

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

946267brestore TransitiveIdeal and TransitiveIdealGraded

@zabrocki

This comment has been minimized.

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 16, 2015

comment:27

I am not convinced that the code in combinat/words from #16553 was meant to be removed and I am leaving it. I've done a search through the documentation and it seems all is removed and all tests pass now.

@zabrocki zabrocki mannequin added the s: needs review label Nov 16, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2015

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

6800765restore RestrictedPartitions, keep because the parts_in does not yet work with restricted length

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2015

Changed commit from 9396e3f to 6800765

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2015

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

9f85070minimize changes to partition.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2015

Changed commit from 6800765 to 9f85070

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 16, 2015

comment:30

I had forgotten that I had not restored the RestrictedPartitions in combinat/partitions.py. Additional functionality needs to be added to Partitions before this can be removed.

@videlec
Copy link
Contributor

videlec commented Nov 23, 2015

comment:31

Apparently, you removed a global import and it causes trouble

**********************************************************************
6 items had failures:
   4 of   5 in sage.combinat.partition.RestrictedPartitions
   4 of   5 in sage.combinat.partition.RestrictedPartitions_nsk.__contains__
   1 of   3 in sage.combinat.partition.RestrictedPartitions_nsk.__init__
   1 of   2 in sage.combinat.partition.RestrictedPartitions_nsk._repr_
   2 of   3 in sage.combinat.partition.RestrictedPartitions_nsk.cardinality
   2 of   3 in sage.combinat.partition.RestrictedPartitions_nsk.list
    [1142 tests, 14 failures, 26.59 s]
----------------------------------------------------------------------
sage -t --long src/sage/combinat/partition.py  # 14 doctests failed
----------------------------------------------------------------------

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2015

Changed commit from 9f85070 to a97a800

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 24, 2015

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

a97a800add import statments in doc-tests

@zabrocki zabrocki mannequin added s: needs review and removed s: needs work labels Nov 24, 2015
@videlec
Copy link
Contributor

videlec commented Nov 26, 2015

comment:34

You removed some deprecation released in sage-6.4 (from 04/14/15)... One year deprecation implies that this should not be merged until 04/14/16 which is in 6 months. This concerns this modification

-        if compact is not None:
-            from sage.misc.superseded import deprecation
-            deprecation(16933, 'compact argument is deprecated.')

@videlec

This comment has been minimized.

@jdemeyer
Copy link

comment:35

Sage 6.4 was released 14 november 2014, so it's more than 1 year ago.

@jdemeyer
Copy link

@jdemeyer
Copy link

comment:37

14 april 2015 is the release date of Sage 6.6

@videlec
Copy link
Contributor

videlec commented Nov 26, 2015

comment:38

Thanks Jeroen for the corrections! I used the dates from trac milestones that were obviously wrong. I fixed them. And I can set this ticket to positive review!

@videlec
Copy link
Contributor

videlec commented Nov 26, 2015

Reviewer: Vincent Delecroix

@vbraun
Copy link
Member

vbraun commented Nov 26, 2015

Changed branch from public/ticket/19513 to a97a800

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

5 participants