New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
random.triangular error when low = high=mode #57564
Comments
When low and mode are the same in random.triangular it gives the following error: <type 'exceptions.ZeroDivisionError'>: float division When high and mode are the same there is no problem. |
I can't reproduce this: Python 2.7.2 |EPD 7.1-2 (32-bit)| (default, Jul 3 2011, 15:40:35)
[GCC 4.0.1 (Apple Inc. build 5493)] on darwin
Type "packages", "demo" or "enthought" for more information.
>>> from random import triangular
>>> triangular(low=1, high=2, mode=1) # low == mode
1.185344240827765 Note that the order of the parameters to random.triangular is (low, high, mode). I suspect that you're actually passing identical values for |
Many thanks, Mark. I'm very new to python so apologies for my obvious mistake (you were absolutely right, I was feeding the high and mode in back to front). As a separate aside, it would be convenient if low=high=mode returned low (or mode or high) rather than error but it's a minor point really and easy to work around as is. Many thanks for your help. |
Yes, I agree that random.triangular should degrade gracefully, in the same way that random.uniform does. |
3.2 doc entry: random.triangular(low, high, mode)
Return a random floating point number N such that low <= N <= high and with the specified mode between those bounds. The low and high bounds default to zero and one. The mode argument defaults to the midpoint between the bounds, giving a symmetric distribution. 3.2 behavior:
>>> from random import triangular
>>> triangular(1,1)
1.0
>>> triangular(1,1,1)
Traceback (most recent call last):
File "<pyshell#2>", line 1, in <module>
triangular(1,1,1)
File "C:\Programs\Python32\lib\random.py", line 346, in triangular
c = 0.5 if mode is None else (mode - low) / (high - low)
ZeroDivisionError: division by zero I regard is as a bug that explicitly giving a 'default value' causes the function to fail. The last sentence of the doc is a lie: the actual default for mode is None: >>> triangular(1,1,None)
1.0 and if it is None, it *not* calculated (low + .5(high-low)). The actual internal third parameter is the fraction of the range (high-low) that is the up slope versus the down slope of the distribution. The code calls that 'c', as calculated by the line shown in the traceback. The fix is simple: add 'or low==high' to the condition. c = 0.5 if (mode is None or low==high) else (mode - low) / (high - low) Contrary to the doc ('mode between those bounds'), the definition on Wikipedia and code include the degenerate cases of mode == low or high. The code in effect treats modes outside the range as being at an endpoint. Suggested doc revision, with defaults given in the signature as normal: random.triangular(low=0.0, high=1.0, mode=None)
Return a random floating point number N from a triangular distribution such that low <= N <= high with the specified mode between or at those bounds. A mode outside a bound is treated as being at the bound. The default mode argument corresponds to the midpoint between the bounds, giving a symmetric distribution. |
I've got this one. |
Here is a patch. |
Where? :-) |
Now it is here. |
Looks fine to me. Raymond: can this be applied? |
One minor comment: I'd prefer it if the second test were "elif low == high:", since that more obviously guards against the division by zero. |
It is written deliberately. What if low == high != mode? |
Then we've got garbage in, garbage out; that case doesn't worry me. |
An exception is better than a garbage result. But I agree, triangular() |
Agreed. And ZeroDivisionError is the wrong exception, too---ValueError would be better. But I'm content that the current patch fixes the immediate issue. |
I'll look at the patch shortly. At first glance, it looks over-engineered to me. |
Added validation of input data. Check whether low <= mode <= high. If low == high return low as a result. |
Test for the issue_13355.patch is incorrect, please fix. |
New changeset c40e36a49033 by Andrew Svetlov in branch '3.3': New changeset 613eb432b152 by Andrew Svetlov in branch 'default': |
New changeset 1062c66e9bdc by Andrew Svetlov in branch '2.7': |
Pushed. |
I haven't had a chance to look at this one yet and am reopening. The triangular code was originally written so that low and high could be reversed and it would still work. I don't want to break any code that might be relying on that. Andrew Svetlov, this patch shouldn't be committed. I'm the maintainer of the random module and the person who wrote the original code for triangular. The bug report was assigned to me to take care of when I got a chance. Please be less aggressive with the commits. |
Andrew Svetlov, please revert the commit. It breaks code that may have been working before the commit. Also, I don't want to change the exceptions being raised in old versions of Python. |
I agree that the issue_13355.patch commit should be reverted: the code used to work fine in the case high < mode < low, and now does not. (Similarly, a call to random.uniform(2.0, 1.0) works as expected at the moment.) Really, I think all that's needed here is Terry's suggested one-line fix. |
New changeset e948154af406 by Andrew Svetlov in branch '3.3': New changeset 39bbbf5d7b01 by Andrew Svetlov in branch 'default': New changeset 620c691c12c5 by Andrew Svetlov in branch '2.7': |
Reverted. Sorry. |
Raymond: have you had time to look at this yet? |
Soonish |
Raymond, could you please make a decision or delegate this issue to Mark, Terry, Andrew or me? |
[Serhiy]
Yes, I will this week. |
I haven't looked at this in depth but it sounds like this is a legitimate concern. I'd like it fixed for 3.4, preferably before rc1. |
Attaching patch |
So now triangular(10, 10, 20) will always return 10? |
While NumPy is of course not normative, this is what they do: >>> numpy.random.triangular(left=1, right=2, mode=0)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "mtrand.pyx", line 3218, in mtrand.RandomState.triangular (numpy/random/mtrand/mtrand.c:13407)
ValueError: left > mode
>>> numpy.random.triangular(left=1, right=2, mode=3)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "mtrand.pyx", line 3220, in mtrand.RandomState.triangular (numpy/random/mtrand/mtrand.c:13433)
ValueError: mode > right
>>> numpy.random.triangular(left=1, right=1, mode=1)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "mtrand.pyx", line 3222, in mtrand.RandomState.triangular (numpy/random/mtrand/mtrand.c:13459)
ValueError: left == right |
Thanks Stefan. For us, I don't see the need to add a restriction, possibly breaking code that is currently working fine (with high < mode <= low). The important part is that we now allow low==mode or high==mode and have a smooth transition to low==high. |
New changeset 7ea6c8eb91e2 by Raymond Hettinger in branch '3.4': |
New changeset 6dc5c4ba7544 by Raymond Hettinger in branch '2.7': |
Note the catch on 2.7. triangular(10, 10.0) returns 10.0, but triangular(10, 10.0, 10.0) returns 10. If then you divide by the result... I proposed change "return low" to "return low + 0.0". |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: