some cleanup in sage.combinat.combinat

[nathanncohen]

Because I hate that :

sage: MultichooseNK(5,3)                                                 
Combinatorial Class -- REDEFINE ME!
   
sage: Partitions(5, min_part=0)
/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/combinat/partition.py:3764: RuntimeWarning: Currently, setting min_part=0 produces Partition objects which violate internal assumptions.  Calling methods on these objects may produce errors or WRONG results!
  warn("Currently, setting min_part=0 produces Partition objects which violate internal assumptions.  Calling methods on these objects may produce errors or WRONG results!", RuntimeWarning)
Partitions of the integer 5 satisfying constraints min_part=0

sage: Compositions(5, min_part=0)
/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/combinat/composition.py:975: RuntimeWarning: Currently, setting min_part=0 produces Composition objects which violate internal assumptions.  Calling methods on these objects may produce errors or WRONG results!
  warn("Currently, setting min_part=0 produces Composition objects which violate internal assumptions.  Calling methods on these objects may produce errors or WRONG results!", RuntimeWarning)
Compositions of the integer 5 satisfying constraints min_part=0

From the help of unordered_tuples (in the global namespace):

Warning: Wraps GAP - hence mset must be a list of objects that have string
representations that can be interpreted by the GAP interpreter.
If mset consists of at all complicated Sage objects, this
function does not do what you expect. A proper function should
be written! (TODO!)

From the help of permutations_iterator (in the global namespace, with no depracation warning):

Do not use this function. It will be deprecated in future version                                                                                                                          
of Sage and eventually removed. Use Permutations instead;

help of number_of_permutations (same as above).

What this ticket does :

• It adds a deprecation warning when min_part = 0, in the hope that the feature will be made unavailable as soon as possible, and because Nicolas told me that I should deprecate it first (turns out that there has been a warning since 2009 already).
• It calls the proper Sage object to compute things that use to be done with GAP
• Some documentation improvements.
• This ticket does nothing about MultichooseNK(5,3) because I refuse to touch anything that uses things like CombinatorialClass.
• This ticket does not remove all the functions number_of_* that are imported in the global namespace by a from sage.combinat.combinat import * because they are almost all deprecated anyway and will be removed, even if we have to wait for one more year.

Nathann

Apply:

• attachment: trac_14138.patch
• attachment: trac_14138-doctests.patch

CC: @nthiery @hivert @hivert @ppurka @tscrim

Component: combinatorics

Author: Nathann Cohen

Reviewer: Punarbasu Purkayastha, Nicolas M. Thiéry

Merged: sage-5.8.beta1

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

[nathanncohen]

comment:2

Il faut vraiment que vous compreniez que ce n'est pas en esperant que "quelqu'un finira par faire le taf un jour" que ces codes pourris vont se réécrire tout seuls. Si tous les gens qui utilisent ces codes savent ou sont les bugs et les contournent, ils n'ont aucune raison de se mettre à améliorer le code (puisqu'on dirait qu'ils ne tirent aucune fierté d'avoir un code propre) et on restera avec ces conneries à vie.

Ce qui personnellement me retient de les recoder, c'est parce que je refuse catégoriquement de mettre les mains dans les catégories. C'est exclusivement ca, parce que je sais que si je mets la main dedans j'aurai besoin de vous sans arret, que je ne serai autonome pour rien et que je devrais me farcir des choix de design avec lesquels je ne suis pas d'accord. Vous faites ce que vous voulez avec votre business, mais moi c'est ca qui me retient de programmer chez vous. Voilà.

Sinon, si ca vous botte de reviewer ces deux trois trucs sur lesquels j'ai passé une petite heure d'allers-retour en discutant avec un pote, ce sera toujours ca de fait.

Nathann

[nathanncohen]

comment:4

(note that the patch changes the behaviour of cyclic_permutations, but as I read in the doctests things like "Note that the behaviour of this function is not very intuitive", I thought of it as an improvement. Tell me what you think of it.)

[ppurka]

comment:7

Hello Nathann, the patch seems good and those files in sage.combinat need a lot of cleanup. They seem abandoned for years :(. I also have some other specific comments:

1. I think the following should be changed to

47	   The following functions will soon be deprecated.

The following functions are deprecated and will soon be removed.

2. The cardinality should be a method

2027	    deprecation(14138, 'Use Combinations(mset,k).cardinality instead.')

deprecation(14138, 'Use Combinations(mset,k).cardinality() instead.')

3. What is this deprecated in favor of? I think it should be mentioned that the min_part=0 will raise an error in the future.

3763	                    from sage.misc.superseded import deprecation 
3764	                    deprecation(14138,"Currently, setting min_part=0 produces "+ 
3765	                                "Partition objects which violate internal "+ 
3766	                                "assumptions. Calling methods on these objects "+ 
3767	                                "may produce errors or WRONG results!") 

4. The following ..note:: should be deleted now since Combinations can handle any Sage object

def number_of_combinations(mset,k):
    """
    Returns the size of combinations(mset,k). IMPLEMENTATION: Wraps
    GAP's NrCombinations.

    .. note::

       ``mset`` must be a list of integers or strings (i.e., this is
       very restricted).

5. The last sentence should be removed here

    Returns the size of arrangements(mset,k). Wraps GAP's
    NrArrangements.

6. In the function def number_of_permutations(mset): I think you can replace all of its code with Permutations(mset).cardinality()?

[nathanncohen]

comment:9

those files in sage.combinat need a lot of cleanup. They seem abandoned for years :(

Yep -_-

I also have some other specific comments:

Done ! Thanks for noticing :-)

Nathann

[nathanncohen]

comment:10

Attachment: trac_14138.patch.gz

[ppurka]

Author: Nathann Cohen

[ppurka]

comment:11

Thanks. Current patch looks good to me and passes all doctests in sage.combinat.

[ppurka]

Reviewer: Punarbasu Purkayastha

[nathanncohen]

comment:12

Thaaaaaaaaaaaaaaaanks ! ;-)

Nathann

[nthiery]

comment:13

Hi Nathann!

Thanks much for the cleanup!

A couple details:

• What's the reason for removing the doctests about
  cyclic_permutations_of_partition and cyclic_permutations?

• Don't deprecate min_part in Partitions. Even if fragile, it is
  useful in some cases, and we will want to support robustly at some
  point. The documentation warns the user.

  Besides, Partitions is under heavy refactoring by Partition options and cleanup partitions documentation #13605 which will
  get into Sage soon; we don't want conflict with that.

• While you are at removing a space in sage/combinat/multichoose_nk.py,
  you might as well remove the comma before :-)

• Please check the discussion on sage-combinat-devel about
  number_of_partitions; I don't remember whether we decided we wanted
  to deprecate it or not.

• ``See http://trac.sagemath.org/14138 for details'' Please use :trac:14138.

  Besides, if the user is referred to the ticket, then the ticket
  should be more explicit not only about what you don't like, but what
  the ticket actually does about it.

Cheers,
Nicolas

[nthiery]

comment:14

Merci d'avoir fait ce nettoyage!

[nathanncohen]

comment:16

Yo.

• What's the reason for removing the doctests about
  cyclic_permutations_of_partition and cyclic_permutations?

I wrote that above. The reason is that the code changed as it now calls what it should have been calling in the first place, and this doctests which says that "the behaviour is not the one that we usually expect" has apprently become what one would expect. Correct it if it is wrong.

• Don't deprecate min_part in Partitions. Even if fragile, it is
  useful in some cases, and we will want to support robustly at some
  point. The documentation warns the user.

I did not want to deprecate min_part but min_part = 0 only. Because IT RETURNS WRONG RESULTS.

Besides, Partitions is under heavy refactoring by #13605 which will
get into Sage soon; we don't want conflict with that.

This ticket was positively reviewed yesterday, your ticket #13605 is 4 months old, is needing a review, weighs 400kb and depends on another ticket #13688 which also needs a review. Why the hell would you delay this one instead and have us work on top of yours ?

• Please check the discussion on sage-combinat-devel about
  number_of_partitions; I don't remember whether we decided we wanted
  to deprecate it or not.

Have fun chatting about what should be done in the future. number_of_partitions has been deprecated by #13072.

• ``See http://trac.sagemath.org/14138 for details'' Please use :trac:14138.

This is no Sphinx code. This is a deprecation warning, automatically generated by the deprecation() function.

Besides, if the user is referred to the ticket, then the ticket
should be more explicit not only about what you don't like, but what
the ticket actually does about it.

What I do not like in this ticket should be obvious to everybody. Just read the code sample and the documentation I quoted.

This ticket has been created, written and reviewed in three days. It is very short. Unless you can give me a fair reason why my patch should depend on yours, which once more is NOT reviewed, is 400kb long and depends on another ticket which still waits for a review please set this ticket back to its initial state.

Nathann

[nathanncohen]

comment:17

Il faut vraiment que tu comprennes qu'on passe aussi notre temps à
nettoyer des trucs et qu'il n'y a que 24 heures dans une journée.

Bullshit. Quand je vous parle de bugs dans votre code, la seule réponse que j'ai jamais eue c'est 'on le fera plus tard', ou "faut voir si le code perso de tout le monde continue à passer". JE fixe les bugs que je trouve dans votre code, et votre code pourri ruine MES calculs. Je fous dans ce projet les codes dont je pense qu'ils peuvent aider d'autres personnes, mais je ne me permets pas d'y foutre des trucs faux qui peuvent induire en erreur des mecs qui essaient de faire leur taf.

Le
monde n'est pas parfait, mais on ne peut pas tout attaquer de
front. Donc on prioritise notre énergie selon ce que l'on juge plus
plus important.

Vous ne prioritisez pas, vous vous foutez de ma gueule. Vous ne corrigez pas les bugs que vous voyez, vous les remettez systematiquement à plus tard, et c'est pour ca que j'écris des patches comme celui-là. Je change ce que je peux changer sans avoir a comprendre vos hierachies insupportables. Sur #14019 il est écrit que Florent s'engageait à corriger le bug des posets sous un mois. C'était le 27 janvier. Si tu devais prendre les paris dessus, tu dirais quoi ? Qu'il est dessus, ou qu'il a oublié ? Il reste une semaine. Je crois que depuis mon départ j'ai du lui balancer une bonne dizaine de mails auxquels je n'ai eu AUCUNE réponse.

J'en ai marre de la politique. Faut corriger ces conneries, et le faire vite. Si vous preferez passer votre temps à discuter c'est cool, mais faut nous laisser faire le taf urgent. Et les résultats faux, c'est prioritaire sur les features.

Nathann

[ppurka]

comment:18

@nthiery: This ticket is mostly orthogonal in functionality to the work in #13605. Most of the changes are in combinat.py which is largely untouched by that ticket. This ticket should not be based on #13605.

About min_part=0 - the warning has been present since early 2009. That's nearly four years. I think it is about time it should either be allowed to have this value 0 in the parameter, or be deprecated. There are no doctests/examples in the file which show when it produces wrong results. If the user has no idea when this gives correct results or when it gives wrong results, it should be removed.

Please set this back to positive review, unless there are more compelling reasons to base it on #13605.

Edit: Refer to correct ticket #13605 instead of #14019

[nthiery]

comment:21

Replying to @ppurka:

@nthiery: This ticket is mostly orthogonal in functionality to the work in #13605. Most of the changes are in combinat.py which is largely untouched by that ticket. This ticket should not be based on #13605.

I totaly agree: it should not depend on #13605! I am just pointing out that, for a minor change that is debatable, it might not be worth creating a conflict.

About min_part=0 - the warning has been present since early 2009. That's nearly four years. I think it is about time it should either be allowed to have this value 0 in the parameter, or be deprecated. There are no doctests/examples in the file which show when it produces wrong results. If the user has no idea when this gives correct results or when it gives wrong results, it should be removed.

As you wish, as long as you promise to support those users that are using this option,
in full knowledge of the risks they were taking, when it will actually be removed.

[nthiery]

comment:22

Replying to @nathanncohen:

I wrote that above. The reason is that the code changed as it now calls what it should have been calling in the first place, and this doctests which says that "the behaviour is not the one that we usually expect" has apprently become what one would expect. Correct it if it is wrong.

Fair enough. I missed the one line change in the code. Thanks for fixing this!

Please add a doctest for the new behavior stating something like: "repetitions are handled properly since #...."

This ticket was positively reviewed yesterday, your ticket #13605 is 4 months old, is needing a review, weighs 400kb and depends on another ticket #13688 which also needs a review. Why the hell would you delay this one instead and have us work on top of yours ?

I never said it should wait for #13605.

• Please check the discussion on sage-combinat-devel about
  number_of_partitions; I don't remember whether we decided we wanted
  to deprecate it or not.

number_of_partitions has been deprecated by #13072.

Perfect. Thanks for checking this out.

• ``See http://trac.sagemath.org/14138 for details'' Please use :trac:14138.

This is no Sphinx code. This is a deprecation warning, automatically generated by the deprecation() function.

Good point.

What I do not like in this ticket should be obvious to everybody. Just read the code sample and the documentation I quoted.

Thanks for adding a description. I might be stupid, but I was missing this information.

This ticket has been created, written and reviewed in three days. It is very short.

You can set it back to positive review as soon as the little things above are resolved. Thanks for handling this in such a prompt manner.

[nathanncohen]

Attachment: trac_14138-doctests.patch.gz

[nathanncohen]

comment:23

Needs a review !

Nathann

[nathanncohen]

comment:24

About min_part=0 - the warning has been present since early 2009. That's nearly four years. I think it is about time it should either be allowed to have this value 0 in the parameter, or be deprecated. There are no doctests/examples in the file which show when it produces wrong results. If the user has no idea when this gives correct results or when it gives wrong results, it should be removed.

As you wish, as long as you promise to support those users that are using this option,
in full knowledge of the risks they were taking, when it will actually be removed.

I can't answer for Punarbasu, but I personally will not. I will not tolerate bugs in this software because 10 guys who know where the code is wrong and do not make any effort to fix it would have to change their personal code.
And there is no reason on earth why I should feel responsible for the backward compatibility of their code when they do not feel responsible at all for the bugs they require us to keep in Sage. This might lead innocents users into very bad situations they have no way to guess.

If it's a problem for you, just create in your branch a patch that reverses the removal of those functions when it will be deleted. Of course that will take time to rebase, and you may have to do this often, and that's a lot of work, but well.. You chosed this development model, didn't you ?

What is for sure is that we never did.

Nathann

[ppurka]

Changed reviewer from Punarbasu Purkayastha to Punarbasu Purkayastha, Nicolas M. Thiéry

[ppurka]

comment:25

Doctests look good to me. Thanks for fixing this. :)

[jdemeyer]

Merged: sage-5.8.beta1

[tscrim]

comment:27

For the record, it took me an hour to rebase #13605 over this patch and #13605 (inadvertently) fixed the issue in partition.py that Nathann wanted to deprecate.

[nathanncohen]

comment:28

For the record, it took me an hour to rebase #13605 over this patch and #13605 (inadvertently) fixed the issue in partition.py that Nathann wanted to deprecate.

Sorry for the trouble. I did not want to have this ticket depend on something else that would be waiting for a review for a year, as #13605 is already 4 months old.

Nathann

[tscrim]

comment:29

Also for the record, #13605 was positively reviewed about the same time as this one.

[nathanncohen]

comment:30

Also for the record, #13605 was positively reviewed about the same time as this one.

This ticket was set to positive_review 5 days ago, when Nicolas Thiery changed it "needs_work" for 4 invalid reasons (you can read it, it is just above) and a new doctest which says "the function does what it should". I have yet to be surprised by the speed at which the combinat code is fixed, so please do not blame me for not believing that your ticket would be reviewed 5 days later, to inadvertently fix a problem which had been laying around for 4 years.

I'm sorry for the trouble it created on your side. I just want to see those problems fixed quickly. And twice, if needed.

Nathann