anneschilling
commented
9 years ago Commit: 3914290
Closed anneschilling closed 9 years ago
anneschilling
commented
9 years ago Commit: 3914290
anneschilling
commented
9 years ago Branch: public/ticket/17898
anneschilling
commented
9 years ago Changed keywords from none to stopgap, partitions
anneschilling
commented
9 years ago New commits:
3914290 | Removed erroneous stopgap |
tscrim
commented
9 years ago LGTM.
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Hello everybody,
Sorry to interrupt, but I believe that what is being done by this code is not advisable for Sage. You make several points in the ticket's description that I will now try to answer.
The function IntegersListsLex returns unexpected results
1) Indeed, as shown on #17637, one can get:
```
sage: Partitions(5, min_slope=1).list()
ValueError: [2, 4] is not a valid partition
sage: IntegerListsLex(5, length=3, max_slope=0).list() # 0 is allowed in the partition
[[5, 0, 0], [4, 1, 0], [3, 2, 0], [3, 1, 1], [2, 2, 1]]
sage: IntegerListsLex(5, max_slope=0).list() # now its not
[[5], [4, 1], [3, 2], [3, 1, 1], [2, 2, 1], [2, 1, 1, 1], [1, 1, 1, 1, 1]]
```
2) Another example comes from the documentation of `IntegersListsLex` itself
```
In the following example, the slope condition is not satisfied
by the upper bound on the parts, and ``[3,3]`` is erroneously
included in the result::
sage: list(IntegerListsLex(6, max_part=3, max_slope=-1))
[[3, 3], [3, 2, 1]]
```Returning wrong results should be considered as a bug
The fact that the docstring of this function reads that "several constraints
must be satisfied by the input, lest the output be incorrect" is not a
sufficient protection.
1) This function can be (and is) called by other functions. Thus, users cannot
be expected to read the documentation of `IntegerListsLex`.
2) In general, a simple line in the documentation in a function is not a
sufficient protection against wrong results. This is the reason why these
'stopgap' tickets exist.
3) It took me a couple of hours to realise that I was not able to write a
code that checks that the input of `IntegerListsLex` is satisfiable. I
take it as a proof that checking it is non-trivial (at least for me). I
cannot expect users to check something that I am not able to check
myself.Some functions which call IntegerListsLex are actually correct
I agree with you that in some cases the `IntegersListsLex` function seems to
return correct results. This is not, however, a sufficient reason to remove a
stopgap whose purpose is to warn users against *possible* wrong results.An alternative way out
As I understand that you may be bothered by those warnings, which appear in
any function that calls `IntegersListsLex` and thus in your code too,
perhaps you could go over some of the use cases of `IntegerListsLex` that
are of interest to you, and only remove the stopgap message after you have
convinced yourself that the function is indeed correct in these situations?
```
if (some_situation_that_was_checked):
pass
else:
<stopgap code>
```
The message would still be shown in other (unchecked) cases, and thus only
in dangerous situations.A long-term fix
As you say in this ticket's description, a real check should be implemented
at the head of `IntegerListsLex`, or the function itself should should be
reimplemented.
I gave the first option a try already, and I remember rather well the knots
it made in my head, as I was going over all ways in which the parameters can
be combined. Checking that they are consistent is nontrivial.
It convinced me that this function already accepts an unhealthy amount
of parameters, and that we will not be able to write a function that handles
all that efficiently and correctly. I believe that we should split this
function into many, that will handle well each combination of parameters
that interests us.
If possible in Cython, for the most useful computations. That would not be a
terribly hard work, in particular for your own code which (I believe)
enumerates only partitions.Nathann
tscrim
commented
9 years ago Bad input is not a bug. Ever.
If you feel that something needs better documentation (like the description of max_slope in Partitions, then put better documentation, not some stopgap which says to everyone that ANY time you use this class, don't expect correct answers. By having this stopgap, you've said just that. It is obviously a horrible thing for Sage.
Also that first example is correct; it's subtle because of the trailing 0's, but it is correct.
You speak of unhealthy amount of stuff into 1 class, but IIRC you've also said that (Di)Graph needs to be one class. If you have a problem with this class, then fix it instead of saying nothing works (which is completely false).
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Hello Travis,
Bad input is not a bug. Ever.
When we are not able to write a piece of code that checks whether the input is bad, I believe that it is a bug.
If you feel that something needs better documentation [...]
I do not. I believe that the wrong results that this function returns are dangerous, and that the users should be warned.
I also proposed that you would only change this code to not display the warning in situations that have been checked for correction, and I would like to know why this does not satisfy you.
Also that first example is correct; it's subtle because of the trailing 0's, but it is correct.
It is an unexpected output. This example is not, however, the only one I provided. Perhaps you will accept the other lines as legit bugs?
You speak of unhealthy amount of stuff into 1 class, but IIRC you've also said that (
Di)Graphneeds to be one class. If you have a problem with this class, then fix it instead of saying nothing works (which is completely false).
This is totally unrelated, but since you ask I highly doubt that I ever suggested that. Indeed, this is the purpose of GenericGraph.
I will write to sage-devel in a second to ask for everybody's advice on this matter. Please, wait for this discussion to take place before setting this ticket back to positive_review.
Nathann
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Sage-devel thread: https://groups.google.com/d/topic/sage-devel/vI8xMUNWCYI/discussion
dimpase
commented
9 years ago Replying to @tscrim:
Bad input is not a bug. Ever.
in your own private code it will only shoot you in the foot.
As it is a non-private function is should check that the input is correct. Otherwise it has potential to do a lot of damage to other people.
I am sure you would scream "Bug!" upon finding an incorrect Python expression that Python runs without any warnings.
anneschilling
commented
9 years ago Replying to @dimpase:
Replying to @tscrim:
Bad input is not a bug. Ever.
in your own private code it will only shoot you in the foot.
As it is a non-private function is should check that the input is correct. Otherwise it has potential to do a lot of damage to other people.
I am sure you would scream "Bug!" upon finding an incorrect Python expression that Python runs without any warnings.
The stopgap that is currently in Sage now pops up all over Sage in situations where IntegerListsLex is used within the range of parameters and hence totally fine. It just scares users unnecessarily. Also, it does not point the user to the documentation where the limitations are listed. Hence the current stopgap in Sage, in my opinion, does more harm than good!
jdemeyer
commented
9 years ago Replying to @anneschilling:
The stopgap that is currently in Sage now pops up all over Sage in situations where IntegerListsLex is used within the range of parameters and hence totally fine. It just scares users unnecessarily. Also, it does not point the user to the documentation where the limitations are listed.
I agree with all of this, but just removing the stopgap isn't the right solution.
nbruin
commented
9 years ago Replying to @tscrim:
Bad input is not a bug. Ever.
But having an interface that declares some input "bad" and other input "good" in a way that makes it hard to decide in which category your input falls is bad design, which is perhaps not a "bug" in the sense that a specification is not followed, but is certainly a "bug" in that it makes the interface very hard to use correctly.
As far as I can see, pretty much arbitrary combinations of the input parameters make sense mathematically. It might just be that they specify an empty or hard (impossible?) to enumerate set. Thus, the routine really only provides a partial implementation of the suggested interface.
There are probably plenty of cases (all the ones where other code calls this routine?) where you know your parameters lie inside the "valid" part of the parameter space. Probably because you know that a certain easily described subset lies in the "valid" part.
As Nathann suggests, just first test if the parameters are "known good". If not, either don't accept the parameters (i.e., raise an error) or print a toned-down warning:
Warning: IntegerListsLex is called with input parameters for which it is not guaranteed to produce correct outputHello Anne,
It just scares users unnecessarily.
I totally agree with that.
I asked in my earlier comment whether you could, instead of removing the stopgap, only raise it in case that have not been checked for correctness. I do not understand why this proposal has been ignored. To me, it sounds like the best way out: you would not see it in case for which you know the code is correct, and we would see it when there is a risk. Isn't it all good for everyone?
Nathann
tscrim
commented
9 years ago Replying to @dimpase:
Replying to @tscrim:
Bad input is not a bug. Ever.
in your own private code it will only shoot you in the foot.
As it is a non-private function is should check that the input is correct. Otherwise it has potential to do a lot of damage to other people.
I am sure you would scream "Bug!" upon finding an incorrect Python expression that Python runs without any warnings.
I would not be screaming bug, but I do agree that it makes it difficult to debug. However the equivalent of this stopgap is to say when Python starts up that "Python may evaluate incorrect expressions and may be unreliable". Would you use Python if that occurred? I wouldn't because I know there are stable languages (or at least claim to be).
@nathanncohen Your proposal is not being ignored, but you have brought about this current state of affairs which needs to be rectified immediately. I am not going to put my time and work into a "hard" problem which I don't see any benefit towards. Instead I think what should be done is have the paths into IntegerListsLex (e.g., in Partitions) check for bad in put (if min_slope >= 0). However the stopgap needs to be removed immediately, and then I will review your implementation.
nbruin
commented
9 years ago Replying to @tscrim:
Bad input is not a bug. Ever.
The documentation is inconsistent on this part for release 6.5. It has one example about "wrong" output
In the following example, the slope condition is not satisfied
by the upper bound on the parts, and ``[3,3]`` is erroneously
included in the result::
sage: list(IntegerListsLex(6, max_part=3, max_slope=-1))
[[3, 3], [3, 2, 1]]However, the conditions mentioned just above:
- The upper and lower bounds themselves should satisfy the
slope constraints.
- The maximal and minimal slopes values should not be equal.
- The maximal and minimal part values should not be equal.don't mention any restrictions on max_part. In fact, it's not documented as possible input at all, so I can't even verify why the output above is wrong:
INPUT:
- ``n`` -- a non negative integer
- ``min_length`` -- a non negative integer
- ``max_length`` -- an integer or `\infty`
- ``length`` -- an integer; overrides min_length and max_length if
specified
- ``floor`` -- a function `f` (or list); defaults to ``lambda i: 0``
- ``ceiling`` -- a function `f` (or list); defaults to
``lambda i: infinity``
- ``min_slope`` -- an integer or `-\infty`; defaults to `-\infty`
- ``max_slope`` -- an integer or `+\infty`; defaults to `+\infty`
An *integer list* is a list `l` of nonnegative integers, its
*parts*. The *length* of `l` is the number of its parts;
the *sum* of `l` is the sum of its parts.and the meaning of max_part and min_part is not documented (not on the effect they have on the generated sequence either)
so ... the stopgap might be screaming a bit loudly, but it doesn't look wrong to me.
tscrim
commented
9 years ago It does violate the assumptions because ceiling defaults to returns max_part, which is constant, whereas the max_slope condition says that all parts must be strictly decreasing. So it is wrong input. While it is not explicitly documented (which should be fixed), this hardly deserves a stopgap.
tscrim
commented
9 years ago I also strongly believe this stopgap message, with all of the different places this shows up across combinatorics, is very harmful to Sage. Moreover #17637 was positively reviewed and merged very quickly (within 2 days IIRC) and as such did not receive the scrutiny that it deserved. As such, this ticket is about returning to the status-quo where we can have a discussion and proposals about a good solution instead of blindly saying things may be wrong.
jdemeyer
commented
9 years ago Replying to @tscrim:
I also strongly believe this stopgap message, with all of the different places this shows up across combinatorics, is very harmful to Sage. Moreover #17637 was positively reviewed and merged very quickly (within 2 days IIRC) and as such did not receive the scrutiny that it deserved. As such, this ticket is about returning to the status-quo where we can have a discussion and proposals about a good solution instead of blindly saying things may be wrong.
In #17637, we followed the correct stopgap procedure: somebody finds a dangerous bug, a stopgap is created to warn people and in the mean time (while the stopgap exists), people can fix the bug.
jdemeyer
commented
9 years ago Replying to @nathanncohen:
I asked in my earlier comment whether you could, instead of removing the stopgap, only raise it in case that have not been checked for correctness. I do not understand why this proposal has been ignored. To me, it sounds like the best way out: you would not see it in case for which you know the code is correct, and we would see it when there is a risk. Isn't it all good for everyone?
I absolutely agree with this. Travis, if you really want to remove the stopgap, just implement the above!
jdemeyer
commented
9 years ago Note this last sentence from the ticket description. The current branch doesn't fit this description:
Replying to @anneschilling:
Instead, either a check should be added to the code to check the user input or the IntegerListsLex code should be extended to allow for all input.
anneschilling
commented
9 years ago Replying to @nathanncohen:
Hello Anne,
It just scares users unnecessarily.
I totally agree with that.
I asked in my earlier comment whether you could, instead of removing the stopgap, only raise it in case that have not been checked for correctness. I do not understand why this proposal has been ignored. To me, it sounds like the best way out: you would not see it in case for which you know the code is correct, and we would see it when there is a risk. Isn't it all good for everyone?
Yes, since you wrote the original stopgap, I suggest that you put in the tests so that the message does not get displayed when the Partition code is used. Also, I would strongly suggest that you add a pointer to the documentation where the limitations of IntegerListsLex is spelled out. Otherwise the stopgap message seems rather useless. At least then the user will know how to use the code!
Thanks,
Anne
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Hello Anne,
Yes, since you wrote the original stopgap, I suggest that you put in the tests so that the message does not get displayed when the Partition code is used.
I know that the Partition code is used in many places, and I expect that your crystal code tests it extensively. I have to say, however, that I do not trust it very much myself: e.g. I wrote #15467 when I noticed that providing values for 'parts_in', 'starting' and 'ending' lead Sage to ignore two among the three.
I believe that this code should be rewritten from scratch, carefully, and more simply (with less combinations of parameters). That would also most definitely lead to speedups.
Also, I would strongly suggest that you add a pointer to the documentation where the limitations of IntegerListsLex is spelled out. Otherwise the stopgap message seems rather useless. At least then the user will know how to use the code!
The description of ticket #17548 can be edited to be more informative to users of Partitions/Crystal. This is where the stopgap currently redirect users.
Nathann
tscrim
commented
9 years ago Replying to @jdemeyer:
Replying to @nathanncohen:
I asked in my earlier comment whether you could, instead of removing the stopgap, only raise it in case that have not been checked for correctness. I do not understand why this proposal has been ignored. To me, it sounds like the best way out: you would not see it in case for which you know the code is correct, and we would see it when there is a risk. Isn't it all good for everyone?
I absolutely agree with this. Travis, if you really want to remove the stopgap, just implement the above!
Why didn't either of you do so to begin with? Instead you both got us into this sad situation. So yet again, I had to spend my time and energy fixing this ridiculous stopgap in order for Sage not to look like it doesn't even work for simple and common combinatorial objects. I am tired of being forced into these things because someone wants to swing a hatchet around on a soapbox.
Thus the issue is now fixed. This may not be the best way, but because this situation, this ticket must go in.
sage: P = Partitions(6, min_slope=0)
sage: list(P)
[[6], [3, 3], [2, 2, 2]]New commits:
b6f375c | Fix known errors and remove the appalling stopgap. |
tscrim
commented
9 years ago Changed branch from public/ticket/17898 to public/combinat/fix_bad_stopgap-17898
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313
commented
9 years ago 7ed8c4ca-6d56-4ae9-953a-41e42b4ed313
commented
9 years ago Branch pushed to git repo; I updated commit sha1. New commits:
2f7a90d | Added a stopgap only when accessed in the global namespace. |
tscrim
commented
9 years ago I've added a stopgap warning for accessing IntegerListsLex from the global namespace, otherwise the code which uses IntegerListsLex as a backend is working as expected AFAIK.
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Hello Travis,
Your current branch removes the stopgap in all cases, despite not properly checking all input (which I do not think can be done). Jeroen and I (if I did not misunderstand his position) stand for something like that:
1) If the set of parameters satisfy constraints for which we know that IntegerListsLex works, then do the job without warning
2) Otherwise, raise a warning
This is the safest path, as we have examples of what you would call bad/misleading input that should not be accepted by this function.
If I understand the intent behind your patch, you believe that IntegerListsLex is used correctly by all of Sage's functions, and that we are only at risk when users call it directly, with possibly wrong input. Thus only the 'public' version of IntegerListsLex will raise the stopgap, and all internal calls will be silent. I believe that it is dangerous too, for many user-exposed functions call IntegerListsLex (and so bypass IntegerListsLexPublic), and may also return wrong output. I am not sure that all internal calls to IntegerListsLex are safe and checked either.
To answer one of your earlier question, I personally did not implement this conditional stopgap because I do not know any restriction of the parameters for which I could swear that only trustworthy results will ever be returned. If you know such a combination and find a reviewer who double-checks it, however, that is a good way out for this ticket.
Nathann
anneschilling
commented
9 years ago Changed author from Anne Schilling to Travis Scrimshaw, Anne Schilling
anneschilling
commented
9 years ago Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Anne Schilling
anneschilling
commented
9 years ago Description changed:
---
+++
@@ -1,4 +1,4 @@
-As documented in https://github.com/sagemath/sage-prod/issues/17548 there are no reported bugs related to IntegerListsLex. All cases listed on that ticket have non-valid input (as documented in the code).
+As documented in https://github.com/sagemath/sage-prod/issues/17548 there are no reported bugs related to IntegerListsLex. All cases listed on that ticket have non-valid input (as documented in the code).
Since the stopgap is not related to any bug and shows up in completely unrelated code, where IntegerListsLex is correctly used, for example
@@ -21,6 +21,5 @@
********************************************************************************
Partitions of 3 having parts less than or equal to 2-it should be removed. +it should be moved to the user interface level, which is done in this ticket.
-Instead, either a check should be added to the code to check the user input or the IntegerListsLex code should be extended to allow for all input.
Hi Nathann,
You agreed that it is very damaging to Sage that a message pops up in many use cases where there is no bug known saying "This code contains bugs and may be mathematically unreliable." The message of the stopgap is also not informative at all saying that this is related to IntegerListsLex. If someone uses Partitions or crystals and this message pops up, they will think it is related to their object, which it is not.
Since this is really a User input issue (and no bugs seem to be known in the valid input range), I think the implemented solution is the best. Also note that the stopgap message does not show up when running tests on the sage tree. Hence your desire to have this message pop up in all cases anyway does not seem to apply for developers (who run tests).
Anne
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Anne, Travis,
Isn't it more important to make sure that our software returns correct results ?
I agree that this warning can be worrying to the users, but I would be even more worried to learn that we start hiding stopgap to pretend that everything is fine when it is not.
We have in Sage a stopgap for a function that does return wrong results, and this is healthy. I do not understand why you keep saying that "there are no bugs" either. This example comes from the documentation, and is a bug to me:
sage: list(IntegerListsLex(6, max_part=3, max_slope=-1))
[[3, 3], [3, 2, 1]]Hiding a warning to protect our "public image" is no responsible way of doing things. And we, developers, need to know when we call a buggy functions, as much as the other users do.
You would not try to hide an error in a fundamental lemma you need in a scientific paper, would you? This is how we mathematicians/computer scientists should be doing our job: with absolute respect for correction and accuracy.
Now, I understand that you may not like to see such a warning raised by your code, and you have in front of you many possibilities to change that:
Please. Let us try to find a way to fix this responsibly, and stop playing this "positive review/needs work" game.
Nathann
jdemeyer
commented
9 years ago Replying to @tscrim:
So yet again, I had to spend my time and energy fixing this ridiculous stopgap in order for Sage not to look like it doesn't even work for simple and common combinatorial objects. I am tired of being forced into these things because someone wants to swing a hatchet around on a soapbox.
"I'm too lazy to fix this bug" is not a valid excuse for not fixing a bug. Sage development is not just about adding new features, sometimes you also need to maintain and improve the quality of existing code.
vbraun
commented
9 years ago I think thats enough discussion for this ticket, at least for now its good enough to warn against IntegerListsLex from public access. Please open a followup for input checking.
Anne, Travis: This question is not just about user checking, but also about catching programming errors. If you want to attract people to the combinat codebase then it mustn't be a pit of broken glass shards that'll cut you if you touch something. There is even the design by contract movement (http://en.wikipedia.org/wiki/Design_by_contract) to turn pre/postcondition checking into the building blocks of software. I know it isn't sexy to sit down for a day and add checking. On the other hand, spending a day later to track down some obscure bug isn't fun either. You can take on technical debt but eventually you'll have to pay. I know, your Partition / Crystal / ... code is perfect and only uses IntegerListsLex in the correct way... I often thought that about my code, too.
vbraun
commented
9 years ago sage -t --long src/doc/en/thematic_tutorials/lie/affine_finite_crystals.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/lie/affine_finite_crystals.rst", line 169, in doc.en.thematic_tutorials.lie.affine_finite_crystals
Failed example:
K = crystals.KirillovReshetikhin(['D',6,1],4,2)
Exception raised:
Traceback (most recent call last):
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 492, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 854, in compile_and_execute
exec(compiled, globs)
File "<doctest doc.en.thematic_tutorials.lie.affine_finite_crystals[0]>", line 1, in <module>
K = crystals.KirillovReshetikhin(['D',Integer(6),Integer(1)],Integer(4),Integer(2))
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 337, in KirillovReshetikhinCrystal
return KashiwaraNakashimaTableaux(cartan_type, r, s)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 390, in KashiwaraNakashimaTableaux
return KR_type_vertical(ct, r, s)
File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1246)
return cls.classcall(cls, *args, **opts)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/affine.py", line 80, in __classcall__
return super(AffineCrystalFromClassical, cls).__classcall__(cls, ct, *args, **options)
File "sage/misc/cachefunc.pyx", line 1298, in sage.misc.cachefunc.WeakCachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:8085)
w = self.f(*args, **kwds)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/structure/unique_representation.py", line 1021, in __classcall__
instance = typecall(cls, *args, **options)
File "sage/misc/classcall_metaclass.pyx", line 518, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:1673)
res = <object> PyType_Type.tp_call(cls, args, opts)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 839, in __init__
KirillovReshetikhinGenericCrystal.__init__(self, cartan_type, r, s)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 461, in __init__
AffineCrystalFromClassical.__init__(self, cartan_type, self.classical_decomposition())
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 978, in classical_decomposition
shapes = vertical_dominoes_removed(self.r(),self.s()))
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 3461, in vertical_dominoes_removed
return [x.conjugate() for x in horizontal_dominoes_removed(s,r)]
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 3475, in horizontal_dominoes_removed
list = [ [y for y in x] + [0 for i in range(r-x.length())] for x in partitions_in_box(r, int(s/2)) ]
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/crystals/kirillov_reshetikhin.py", line 3445, in partitions_in_box
return [x for n in range(r*s+1) for x in Partitions(n,max_part=s,max_length=r)]
File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1246)
return cls.classcall(cls, *args, **opts)
File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/partition.py", line 4808, in __classcall_private__
if kwargs['min_slope'] > 0:
KeyError: 'min_slope'
**********************************************************************Branch pushed to git repo; I updated commit sha1. New commits:
d3de7cf | Fix a stupid bug. |
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313
commented
9 years ago tscrim
commented
9 years ago @nathanncohen You also wouldn't use a lemma if you don't satisfy its hypotheses, and if your situation doesn't, then it doesn't make the lemma wrong. It is also our responsibility to display warnings/errors when it makes sense.
@jdemeyer "I'm too lazy to figure out a good way to display a warning" is how we got into this situation.
@vbraun There's a sign along the path telling you where the shards are way off the trail, but I don't think we want a sign saying you don't want to walk in the forest. I agree this should be more visible for users at the frontend (and I forgot this class was in the global namespace), but I don't agree with getting angry calls from debt collectors well before it comes due.
nbruin
commented
9 years ago Replying to @tscrim:
@vbraun There's a sign along the path telling you where the shards are way off the trail,
I assume you're talking about the docstring. Before I wrote [comment:13] I seriously tried looking at the specifications of the functions to see if I could find some admissible subset of the parameter space for which membership was easy to check, because I suspected that nearly all practical use of the routine picks parameters from this easily tested space (after all, when the code was written someone checked the parameters would always be valid and it seems unlikely they were able to do so using some deep result). I got stuck immediately because the first example with claimed inadmissible parameters relies on parameters that aren't even documented. So from experience I know the documentation does not describe properly which parameter values are admissible or not.
nbruin
commented
9 years ago There's a problem with stopgap messages, especially when they are only printed for some parameter choices and/or get triggered internally, via a route that may not be easily derived by the user:
So, I think the message in practice is actually less informative than one would initially think, especially when it's printed conditionally. Extra reason to resolve this ticket quickly and properly, and get to a situation where the routine either returns correct answers (for a reasonable interpretation of the input) or raises an error. Stopgaps are really a very poor substitute for that.
jdemeyer
commented
9 years ago Replying to @nbruin:
Stopgaps are really a very poor substitute for that.
Stopgaps were never meant as a substitute for that. Stopgaps are used in situations where a bug has been found but the bug is non-trivial to fix. Then a stopgap message is put into place to warn users until the bug is fixed.
Really, that's also what we should do here: fix #17548 first and only then remove the stopgap message.
6bdad4c1-1e26-4f2f-a442-a01a2292c181
commented
9 years ago Really, that's also what we should do here: fix #17548 first and only then remove the stopgap message.
+1. What we are doing here is dangerous for everybody's computations, developers as well as regular users.
We should not lower our standards when it comes to wrong results, and I do not think that this ticket should be accepted.
Nathann
jdemeyer
commented
9 years ago (never mind, forgot to restart Sage after recompiling)
jdemeyer
commented
9 years ago The bug in comment [comment:13] still manifests itself in Compositions:
sage: Compositions(6, max_part=3, max_slope=-1).list()
[[3, 3], [3, 2, 1]]When playing with these functions, it's easy to get wrong results. In the example below, the numbers don't even sum to 4:
sage: Compositions(4, length=3, min_slope=1).list()
[[1, 2, 4]]This should be the empty set:
sage: Partitions(6, outer=[4,2], inner=[3,3]).list()
[[3, 3]]The first one is documented:
However, providing incoherent constraints may yield strange
results. It is up to the user to ensure that the inner and outer
compositions themselves satisfy the parts and slope constraints.
[snip]
The generation algorithm is constant amortized time, and handled
by the generic tool :class:`IntegerListsLex`.The second one is a bug, and should be fixed on a followup ticket. I will fix that on a followup ticket (because I know neither you nor Nathann will). The third violates the assumptions.
@nathanncohen I don't think the original stopgap should have been accepted because your standards were too low for making a good set of conditions.
Currently the stopgap on IntegerListsLex pops up in many functions and classes, where IntegerListsLex is used in the correct parameter range. This ticket moves the stopgap to the user level and implements further checks on the validity of the input parameters for IntegerListsLex.
Here are examples where the message currently appears without reference to IntegerListsLex:
and
CC: @tscrim @nthiery @videlec @jdemeyer
Component: combinatorics
Keywords: stopgap, partitions
Author: Travis Scrimshaw, Anne Schilling
Branch:
ec3405eReviewer: Travis Scrimshaw, Anne Schilling
Issue created by migration from https://trac.sagemath.org/ticket/17898