sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 478 forks source link

remove expired deprecations in sage/combinat #19513

Closed d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db closed 8 years ago

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

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

jdemeyer commented 9 years ago
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.

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
comment:2

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

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-A grep of the word `deprecation` in the source code indicates that there are many functions which have expired and may be removed.  We will remove those deprecations which have been in Sage for at least one year.
+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).
f29946bc-ee7b-48cd-9abc-3445948c551d commented 9 years ago
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.)

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
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.

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Branch: public/ticket/19513

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Changed keywords from none to deprecate

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Commit: 18dd976

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
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
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Author: Mike Zabrocki

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
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

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 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: #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920, #16553
tscrim commented 9 years ago
comment:7

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

jdemeyer commented 9 years ago
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.

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
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.

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 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: #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920, #16553
+Code deprecated from: #6637, #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920, #16553
tscrim commented 9 years ago
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 commented 9 years ago
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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 18dd976 to 946267b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

946267brestore TransitiveIdeal and TransitiveIdealGraded
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 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: #6637, #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920, #16553
+Code deprecated from: #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920, #16553
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
comment:13

I haven't done a test all yet, but I did find them in sage/categories. They can be deprecated and removed in other tickets.

jdemeyer commented 9 years ago
comment:14

You can still remove TransitiveIdeal and friends from src/sage/combinat/all.py (that's the point of the deprecated_callable_import).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 946267b to e403dda

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

e403ddaremove deprecations in all.py to TransitiveIdeal* and SearchForest, line from doc/en/reference/combinat/module_list.rst
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 9 years ago
comment:16

I also restored the comment (that was removed in #16352 because it looked like it had been addressed) in backtrack.py that there is an intention that TransitiveIdeal and TransitiveIdealGraded should eventually be deprecated. At least TransitiveIdeal is still used in sage/categories/finitely_generated_semigroups.py and so it can't be deleted completely. I'd start the deprecation here, but that runs contrary to the purpose of this ticket.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from e403dda to bf792dd

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

8069b74add pickle overrides
bf792ddrestored words/alphabet.py #8920 as removing the code causes errors in structure/sage_object
darijgr commented 8 years ago
comment:18

Branch went red (just saying).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from bf792dd to 472c3b0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

472c3b0fix combinat/enumerated_sets.py
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 8 years ago
comment:20

That last commit was a merge with develop.

I'm not quite sure what should be done with the deprecated code in words/alphabet.py from #8920 since deleting that code raises unpickling errors.

tscrim commented 8 years ago
comment:21

Replying to @zabrocki:

I'm not quite sure what should be done with the deprecated code in words/alphabet.py from #8920 since deleting that code raises unpickling errors.

We probably could setup a register_unpickle_override going to a slightly tweaked OrderedAlphabet_backward_compatibility to handle those old pickles.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 472c3b0 to 2b6c11a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

2b6c11alet's have a few doctests back
darijgr commented 8 years ago
comment:23

1) Why are we deprecating RestrictedPartitions? From the way it looks, this functionality is not implemented elsewhere in its full strength (i.e., fixing both a set and a length).

2) Why are we undeprecating TransitiveIdeal? Not deleting it is fine, but removing the deprecation warnings?

3) I have just realized that CyclicPermutations of a multiset misbehave: e.g., the cardinality method is inherited from Permutations and thus disagrees with the list method. What should we do about this?

4) Another lost child:

sage: Permutations(["c","a","t"])[0]
['c', 'a', 't']
sage: _ in Permutations()
False

(Yet, the class of Permutations(["c","a","t"])[0] inherits from Permutations.)

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 8 years ago
comment:24

Replying to @darijgr:

1) Why are we deprecating RestrictedPartitions? From the way it looks, this functionality is not implemented elsewhere in its full strength (i.e., fixing both a set and a length).

You may be right about this. There is a warning note that I may not have heeded carefully and I can restore those functions if the current Partitions function doesn't have this functionality. I will check carefully and restore it if need be.

2) Why are we undeprecating TransitiveIdeal? Not deleting it is fine, but removing the deprecation warnings?

The deprecation warnings were only an indication that the function is being removed from the global name space since there were previously no deprecation warnings if you just imported the functions and started using them. I restored a comment that TransitiveIdeal and TransitiveIdealGraded do need to be deprecated.

3) I have just realized that CyclicPermutations of a multiset misbehave: e.g., the cardinality method is inherited from Permutations and thus disagrees with the list method. What should we do about this?

4) Another lost child:

sage: Permutations(["c","a","t"])[0]
['c', 'a', 't']
sage: _ in Permutations()
False

(Yet, the class of Permutations(["c","a","t"])[0] inherits from Permutations.)

Yes. I agree that both of these are problems. I tested:

sage: len(CyclicPermutations(range(4)))==CyclicPermutations(range(4)).cardinality()
False
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 8 years ago
comment:25

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

No problem with the current modifications to the design files. Thanks for doing this, by the way.

Nathann

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

9396e3fMerge branch 'develop' into public/ticket/19513
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 2b6c11a to 9396e3f

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 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: #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920, #16553
+Code deprecated from: #6637, #14101, #14140, #14772, #16472, #15142, #14496, #15473, #14673, #13872, #15882, #8920
d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 8 years ago
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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 9396e3f to 6800765

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

9f85070minimize changes to partition.py
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 6800765 to 9f85070

d4d9e38a-6e64-40d7-a7f7-bd828eb9e0db commented 8 years ago
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 commented 8 years ago
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
----------------------------------------------------------------------
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 9f85070 to a97a800

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

a97a800add import statments in doc-tests