Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#14138 closed defect (fixed)

some cleanup in sage.combinat.combinat

Reported by: ncohen Owned by: sage-combinat
Priority: major Milestone: sage-5.8
Component: combinatorics Keywords:
Cc: nthiery, fhivert, hivert, ppurka, tscrim Merged in: sage-5.8.beta1
Authors: Nathann Cohen Reviewers: Punarbasu Purkayastha, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ncohen)

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:

Attachments (2)

trac_14138.patch (18.7 KB) - added by ncohen 7 years ago.
trac_14138-doctests.patch (1.5 KB) - added by ncohen 7 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 7 years ago by ncohen

  • Cc nthiery fhivert hivert added
  • Description modified (diff)

comment:2 follow-up: Changed 7 years ago by ncohen

  • Status changed from new to needs_review

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

comment:3 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:4 Changed 7 years ago by ncohen

(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.)

comment:5 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:6 Changed 7 years ago by ppurka

  • Cc ppurka added

comment:7 follow-up: Changed 7 years ago by ppurka

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()?

comment:8 Changed 7 years ago by ppurka

  • Status changed from needs_review to needs_work

comment:9 in reply to: ↑ 7 Changed 7 years ago by ncohen

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

Changed 7 years ago by ncohen

comment:10 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:11 Changed 7 years ago by ppurka

  • Authors set to Nathann Cohen
  • Reviewers set to Punarbasu Purkayastha
  • Status changed from needs_review to positive_review

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

comment:12 Changed 7 years ago by ncohen

Thaaaaaaaaaaaaaaaanks ! ;-)

Nathann

comment:13 follow-up: Changed 7 years ago by nthiery

  • Status changed from positive_review to needs_work

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 #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.

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

comment:14 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by nthiery

Merci d'avoir fait ce nettoyage!

Last edited 7 years ago by nthiery (previous) (diff)

comment:15 Changed 7 years ago by tscrim

  • Cc tscrim added

comment:16 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by ncohen

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.

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

comment:17 in reply to: ↑ 14 Changed 7 years ago by ncohen

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

comment:18 follow-up: Changed 7 years ago by 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.

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

Last edited 7 years ago by ppurka (previous) (diff)

comment:19 Changed 7 years ago by ncohen

  • Description modified (diff)

comment:20 Changed 7 years ago by ncohen

  • Description modified (diff)

comment:21 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by nthiery

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.

comment:22 in reply to: ↑ 16 Changed 7 years ago by nthiery

Replying to ncohen:

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.

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.

Changed 7 years ago by ncohen

comment:23 Changed 7 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Needs a review !

Nathann

comment:24 in reply to: ↑ 21 Changed 7 years ago by ncohen

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

comment:25 Changed 7 years ago by ppurka

  • Reviewers changed from Punarbasu Purkayastha to Punarbasu Purkayastha, Nicolas M. Thiéry
  • Status changed from needs_review to positive_review

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

comment:26 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.8.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 follow-up: Changed 7 years ago by tscrim

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.

comment:28 in reply to: ↑ 27 Changed 7 years ago by ncohen

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

comment:29 follow-up: Changed 7 years ago by tscrim

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

comment:30 in reply to: ↑ 29 Changed 7 years ago by ncohen

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

Note: See TracTickets for help on using tickets.