Ticket #13742 (closed defect: fixed)
No Permutation should be created that its method cannot handle
|Reported by:||ncohen||Owned by:||sage-combinat|
|Cc:||vdelecroix, hivert, nthiery, dimpase, tscrim||Work issues:|
|Report Upstream:||N/A||Reviewers:||Dmitrii Pasechnik|
|Authors:||Nathann Cohen||Merged in:||sage-5.6.beta1|
Description (last modified by dimpase) (diff)
The code from sage.combinat.permutation is very bad. Bad input is never checked, and when ones decides that Permutations are only defined on integers and do not contain 0, bad input happens. The code implicitly assumes that the elements are 1...n despite that, and the robinson_schensted function (no idea of what it does) definitely uses it on something different (i.e. lists, on which no total order is even defined !).
This patch was meant to convince me that Sage could be trusted, and after wasting several hours on this I just *DO NOT* trust what is contained in the Combinat branch. It has been explained to me that "the code depends on CombinatorialObject?? which is deprecated -- not as we usually deprecate code, but by comments in its documentation -- and that it could not be deprecated formally (with warnings, exceptions, ...) because a lot of people use it." And it seems to have been the case for the last 5 years.
Nothing is done to change that. And I will not. This patch is an attempt to convince me that I can use the Permutation class. That is all. It checks stuff, adds warnings everywhere, and documentation.
Honestly, it makes me sick to think such a thing is in Sage. That's bad work, period.
Two comments :
- For those who want to use the code as before, even when to handle things it is not meant to handle, use check_input=True as a flag when creating a partition Object. And if you NEED to use this, you may as well write the code you need properly. Don't share code that you cannot trust.
- from_cycles considers that the empty permutation is , while Permutation() is . It does not make any sense. I'm sick of it. This is bad work too. I don't care what people prefer, but please take a decision and stick with it.
Apply to the Sage library
- Status changed from new to needs_review
- Authors set to Nathann Cohen
- Status changed from needs_review to needs_info
- Description modified (diff)
comment:10 follow-up: ↓ 11 Changed 7 months ago by ncohen
- Status changed from needs_info to needs_review
comment:14 follow-up: ↓ 15 Changed 7 months ago by dimpase
- Status changed from needs_review to needs_work