Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17898 closed defect (fixed)

Move of stopgap to user level

Reported by: aschilling Owned by:
Priority: major Milestone: sage-6.6
Component: combinatorics Keywords: stopgap, partitions
Cc: tscrim, nthiery, vdelecroix, jdemeyer Merged in:
Authors: Travis Scrimshaw, Anne Schilling Reviewers: Travis Scrimshaw, Anne Schilling
Report Upstream: N/A Work issues:
Branch: ec3405e (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by aschilling)

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?:

sage: K = crystals.KirillovReshetikhin(['D',4,1],1,1)
/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/partition.py:4827:
********************************************************************************
This code contains bugs and may be mathematically unreliable.
This issue is being tracked at http://trac.sagemath.org/sage_trac/ticket/17548.
********************************************************************************

and

sage: Partitions(3,max_part=2)
/Applications/sage/local/lib/python2.7/site-packages/sage/combinat/partition.py:6622:
********************************************************************************
This code contains bugs and may be mathematically unreliable.
This issue is being tracked at http://trac.sagemath.org/sage_trac/ticket/17548.
********************************************************************************
Partitions of 3 having parts less than or equal to 2

Change History (65)

comment:1 Changed 4 years ago by aschilling

  • Branch set to public/ticket/17898
  • Commit set to 39142901893bc0207e8271ccd4772469fe958e0f
  • Keywords stopgap partitions added
  • Status changed from new to needs_review

New commits:

3914290Removed erroneous stopgap

comment:2 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:3 Changed 4 years ago by ncohen

  • Cc vdelecroix jdemeyer added
  • Status changed from positive_review to needs_work

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

Last edited 4 years ago by ncohen (previous) (diff)

comment:4 follow-ups: Changed 4 years ago by tscrim

  • Status changed from needs_work to positive_review

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

comment:5 in reply to: ↑ 4 Changed 4 years ago by ncohen

  • Status changed from positive_review to needs_work

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

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

Last edited 4 years ago by ncohen (previous) (diff)

comment:7 in reply to: ↑ 4 ; follow-ups: Changed 4 years ago by 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.

comment:8 in reply to: ↑ 7 ; follow-ups: Changed 4 years ago by aschilling

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!

comment:9 in reply to: ↑ 8 Changed 4 years ago by jdemeyer

Replying to aschilling:

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.

comment:10 in reply to: ↑ 4 Changed 4 years ago by nbruin

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 output

comment:11 in reply to: ↑ 8 ; follow-ups: Changed 4 years ago by ncohen

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?

Nathann

comment:12 in reply to: ↑ 7 Changed 4 years ago by tscrim

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

@ncohen 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.

comment:13 in reply to: ↑ 4 Changed 4 years ago by nbruin

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.

comment:14 Changed 4 years ago by tscrim

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.

comment:15 follow-up: Changed 4 years ago by tscrim

  • Status changed from needs_work to positive_review

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.

comment:16 in reply to: ↑ 15 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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.

comment:17 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

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!

comment:18 in reply to: ↑ description Changed 4 years ago by jdemeyer

Note this last sentence from the ticket description. The current branch doesn't fit this description:

Replying to aschilling:

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.

comment:19 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by aschilling

Replying to ncohen:

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

comment:20 in reply to: ↑ 19 Changed 4 years ago by ncohen

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

comment:21 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by tscrim

  • Branch changed from public/ticket/17898 to public/combinat/fix_bad_stopgap-17898
  • Commit changed from 39142901893bc0207e8271ccd4772469fe958e0f to b6f375ce349643dbff01ba8cb4c86ffc5a446e0e

Replying to jdemeyer:

Replying to ncohen:

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:

b6f375cFix known errors and remove the appalling stopgap.

comment:22 Changed 4 years ago by git

  • Commit changed from b6f375ce349643dbff01ba8cb4c86ffc5a446e0e to 2f7a90d8419ca8d2202b3cb31290e58194f666e3

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

2f7a90dAdded a stopgap only when accessed in the global namespace.

comment:23 Changed 4 years ago by tscrim

  • Status changed from needs_work to needs_review

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.

comment:24 follow-up: Changed 4 years ago by ncohen

  • Status changed from needs_review to needs_info

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

Last edited 4 years ago by ncohen (previous) (diff)

comment:25 Changed 4 years ago by aschilling

  • Authors changed from Anne Schilling to Travis Scrimshaw, Anne Schilling
  • Description modified (diff)
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Anne Schilling
  • Status changed from needs_info to needs_review

comment:26 in reply to: ↑ 24 ; follow-up: Changed 4 years ago by aschilling

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

comment:27 Changed 4 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:28 in reply to: ↑ 26 Changed 4 years ago by ncohen

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:

  • Include the name of the faulty function is the stopgap warning: "The function IntegerListsLex? is knows to return wrong result [...]". It would actually be a nice improvement to stopgap in general.
  • Not show the warning for inputs that have been checked to be correct
  • Rewrite a small function to enumerate only the objects that you need, i.e. without having to handle all the parameters at once.

Please. Let us try to find a way to fix this responsibly, and stop playing this "positive review/needs work" game.

Nathann

comment:29 in reply to: ↑ 21 Changed 4 years ago by jdemeyer

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.

comment:30 Changed 4 years ago by vbraun

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.

comment:31 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
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'
**********************************************************************

comment:32 Changed 4 years ago by git

  • Commit changed from 2f7a90d8419ca8d2202b3cb31290e58194f666e3 to d3de7cf960cb38e03d69d3e4b8951bcc9ddd830a

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

d3de7cfFix a stupid bug.

comment:33 follow-up: Changed 4 years ago by tscrim

  • Status changed from needs_work to positive_review

@ncohen 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.

comment:34 in reply to: ↑ 33 Changed 4 years ago by nbruin

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

comment:35 follow-up: Changed 4 years ago by nbruin

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:

  • the message only gets printed, so it may easily get lost in other output
  • the message only gets printed once. The first time this happens, the user can (at least in theory) know where they put unguaranteed parameters in. After that, however, the protection is lost: both valid and invalid parameters lead to the same (silent) behaviour.

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.

comment:36 in reply to: ↑ 35 ; follow-up: Changed 4 years ago by jdemeyer

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.

comment:37 in reply to: ↑ 36 Changed 4 years ago by ncohen

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

comment:38 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

(never mind, forgot to restart Sage after recompiling)

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:39 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:40 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The bug in comment 13 still manifests itself in Compositions:

sage: Compositions(6, max_part=3, max_slope=-1).list()
[[3, 3], [3, 2, 1]]

comment:41 Changed 4 years ago by jdemeyer

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]]

comment:42 Changed 4 years ago by jdemeyer

This should be the empty set:

sage: Partitions(6, outer=[4,2], inner=[3,3]).list()
[[3, 3]]

comment:43 follow-up: Changed 4 years ago by tscrim

  • Status changed from needs_work to positive_review

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.

@ncohen I don't think the original stopgap should have been accepted because your standards were too low for making a good set of conditions.

comment:44 Changed 4 years ago by ncohen

  • Status changed from positive_review to needs_work

Let us make things clear: you will *NOT* remove a stopgap from a function which returns this as a list of partitions of 4.

sage: list(IntegerListsLex(4,length=3,min_slope=1))
[[1, 2, 4], [0, 3, 4], [0, 2, 5], [0, 1, 6]]

You cannot, by yourself, set a ticket to positive review against everybody's advice (and common sense).

Nathann

P.S. : the 'public' copy of IntegerListsLex, called by the users, contains absolutely no documentation. This branch is terrible work, period.

Last edited 4 years ago by ncohen (previous) (diff)

comment:45 Changed 4 years ago by ncohen

The description of this ticket also needs to be updated: it claims that there are no reported bug in IntegerListsLex.

comment:46 in reply to: ↑ description Changed 4 years ago by jdemeyer

Replying to aschilling:

All cases listed on that ticket have non-valid input (as documented in the code).

This is no longer true, so it removes the excuse to remove the stopgap.

comment:47 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:48 in reply to: ↑ 43 ; follow-up: Changed 4 years ago by jhpalmieri

I hate to get involved in this because the level of discourse is not up to Sage's usual standards.

Replying to tscrim:

The first one is documented:

This is not actually clear to me. I think by "the first one" you mean Compositions(6, max_part=3, max_slope=-1).list(), but the note could be viewed as just discussing inner and outer partitions which have not been explicitly specified. The note is not very prominent, in any case, and other similar cases have no such accompanying warning in the documentation:

sage: Compositions(6, max_part=3, max_slope=-1).list() # "documented"
[[3, 3], [3, 2, 1]]
sage: list(IntegerVectors(6, max_part=3, max_slope=-1, min_part=1)) # not documented: a bug
[[3, 3], [3, 2, 1]]

Is length a valid option for IntegerVectors?

sage: list(IntegerVectors(4, length=3, min_part=1)) # a bug?
[[4], [3, 1], [2, 2], [2, 1, 1], [1, 3], [1, 2, 1], [1, 1, 2], [1, 1, 1, 1]

And as an aside, something for a separate ticket, there is

sage: list(IntegerVectors(6, max_part=3, max_slope=-1, min_part=1))
[[3, 3], [3, 2, 1]]
sage: IntegerVectors(6, max_part=3, max_slope=-1, min_part=1).list()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-50-a7a7eaaa2968> in <module>()
----> 1 IntegerVectors(Integer(6), max_part=Integer(3), max_slope=-Integer(1), min_part=Integer(1)).list()

/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/integer_vector.pyc in list(self)
   1126         """
   1127         if 'max_length' not in self.constraints:
-> 1128             raise NotImplementedError("infinite list") # no list from infinite iter
   1129         else:
   1130             return list(self)

NotImplementedError: infinite list

Why does list(...) work differently from (...).list?

Anyway, the fact that the documentation for IntegerVectors lists none of the options is not acceptable. The docstring includes the definition IntegerVectors(n=None, k=None, **kwargs), and not only does it list none of the keyword arguments, it doesn't even say what n and k mean. As has already been pointed out, the documentation for IntegerListsLex doesn't list max_part in its possible inputs. The documentation for Compositions doesn't list any of the inputs. It does say See also "Composition", "Partitions", "IntegerVectors", the documentation of one of which discusses the options. (It should probably say See "Partitions" for a description of the available options, or something like that.) The state of the documentation is terrible, and it seems clear that whoever wrote and reviewed this code was not thinking about how anyone would actually use it.

(I also think that any docstring that discusses slope should define it the way the docstring for IntegerListsLex does (explicitly saying l[i+1]-l[i]) rather than the way the docstring for Partitions does (the difference between successive parts, because that could mean l[i+1]-l[i] or its negative or possibly its absolute value). This is made more confusing by an example in the Partitions documentation about a partition whose "parts differ by at least 2", which sounds like min_slope=2, but is actually max_slope=-2.)

From this point of view, the idea of a stopgap is pretty mild: we could actually be discussing ripping all of this code out of Sage until its documentation, and probably its input checking, is in good shape. We shouldn't rip it out, of course, but this part of Sage definitely needs work.

comment:49 Changed 4 years ago by jhpalmieri

Rephrasing: rather than watching this train wreck from a distance with my binoculars, I've decided to get a closer view by parking my car on the tracks.

comment:50 Changed 4 years ago by git

  • Commit changed from d3de7cf960cb38e03d69d3e4b8951bcc9ddd830a to c4063bde960d2c8e7216e733190293af456d143d

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

02413efImplement connected components for posets.
d27121bAdding method to header information.
0bd5971Merge branch 'public/combinat/fix_bad_stopgap-17898' of trac.sagemath.org:sage into public/combinat/fix_bad_stopgap-17898
c4063bdMany fixes and error checking to appease.

comment:51 Changed 4 years ago by git

  • Commit changed from c4063bde960d2c8e7216e733190293af456d143d to f7e2d10938ed6c8ba4f448ff3fde2baae5112642

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f7e2d10Many fixes and error checking to appease.

comment:52 follow-up: Changed 4 years ago by tscrim

  • Status changed from needs_work to needs_review

Last commit fixes some bad branch management on my part. *ahem*

So after some more thought, Jeroen's second example is actually bad input because the floor condition does not satisfy the min_slope condition, so it is not a bug in that sense. That is what I get for trying to do this as I'm running out the door.

Now my most recent changes does some fixing and hacking to better handle things, now it raises some errors on bad input, and has documentation for the IntegerListsLexPublic class (I thought python simply inherited the docstring like sphinx does). This is more than a fair compromise and allows Sage to not scare away people from more common functionality.

@ncohen First, look at what you wrote. Second, I just reverted back to positive review in accordance with my viewpoint. Now I can't make you be a part of the solution, but I'd appreciate it if stop throwing mud on our house and expecting us to clean it up by ourselves. Do I have to get Barney singing here?

comment:53 in reply to: ↑ 52 Changed 4 years ago by ncohen

@ncohen First, look at what you wrote. Second, I just reverted back to positive review in accordance with my viewpoint. Now I can't make you be a part of the solution, but I'd appreciate it if stop throwing mud on our house and expecting us to clean it up by ourselves. Do I have to get Barney singing here?

I wish you did not get the impression that I try to make your life more difficult. When I began worrying over the state of Partitions, didn't I write to Anne and you a very long email (on the 15/01/15) about what it meant for crystals? I am not doing anything tricky, nor complicated: when I see a bug I try to patch it, and when I do not know how I write a stopgap. I did not think that it could cause so much trouble.

Nathann

comment:54 in reply to: ↑ 48 Changed 4 years ago by aschilling

From this point of view, the idea of a stopgap is pretty mild: we could actually be discussing ripping all of this code out of Sage until its documentation, and probably its input checking, is in good shape. We shouldn't rip it out, of course, but this part of Sage definitely needs work.

John, I completely agree with you, this code is not in good shape. Instead of implementing input checking, I think one should probably rewrite it from scratch. However, this ticket is about the stopgap itself. The current message it displays is not really useful, especially when it pops up in code that uses IntegerListsLex? in seemingly valid regions. The message does not even say that the culprit is IntegerListsLex? or points the user to the documentation. That's what I thought this ticket should address. I agree in the long run some of us should rewrite IntegerListsLex? completely!

The good thing is that this whole discussion brings up many of the issues with the IntegerListsLex? code.

Best,

Anne

comment:55 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Things are improving, but we still have this bug:

sage: Partitions(6, outer=[4,2], inner=[3,3]).list()
[[3, 3]]

and this now hangs forever:

sage: Partitions(6, max_part=3, max_slope=-4).list()

comment:56 Changed 4 years ago by jdemeyer

And before you claim that the above examples are "wrong input": Partitions doesn't have any documented conditions on the input arguments.

comment:57 Changed 4 years ago by git

  • Commit changed from f7e2d10938ed6c8ba4f448ff3fde2baae5112642 to 26c52361a0e302219c30736861276901717abbce

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

26c5236Some more fixes.

comment:58 Changed 4 years ago by git

  • Commit changed from 26c52361a0e302219c30736861276901717abbce to ec3405e8913eba4c7ce55b7abe69a4829144a844

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

4f4578bAdded another doctest.
ec3405eTweaked the doctest.

comment:59 Changed 4 years ago by tscrim

  • Status changed from needs_work to needs_review

Fixed.

comment:60 follow-up: Changed 4 years ago by aschilling

  • Description modified (diff)
  • Status changed from needs_review to positive_review
  • Summary changed from Removal of wrong stopgap to Move of stopgap to user level

comment:61 in reply to: ↑ 60 Changed 4 years ago by aschilling

This looks good to me now.

Regarding John's comments about IntegerVectors?: as far as I can see IntegerVectors? does not use IntegerListsLex?. Hence someone should implement a stopgap for that class separately or fix up the code!! I agree that it is in bad shape. Nathann might know more since he has contributed to that file. I suggest to open a new ticket regarding IntegerVectors?.

comment:62 Changed 4 years ago by vbraun

  • Branch changed from public/combinat/fix_bad_stopgap-17898 to ec3405e8913eba4c7ce55b7abe69a4829144a844
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:63 Changed 4 years ago by ncohen

  • Commit ec3405e8913eba4c7ce55b7abe69a4829144a844 deleted

The problem with IntegerVectors is addressed in #17927.

Nathann

comment:64 Changed 4 years ago by jdemeyer

The functions Partitions and Compositions still need the stopgap, see #17956.

comment:65 Changed 4 years ago by jdemeyer

You actually documented a bug in a doctest:

        Check that :trac:`17898` is fixed::

            sage: P = Partitions(5, min_slope=0)
            sage: list(P)
            [[5]]

Where is [1, 1, 1, 1, 1]???

Note: See TracTickets for help on using tickets.