Skip to content
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

Closed
mark108 mannequin opened this issue Nov 6, 2011 · 38 comments
Closed

random.triangular error when low = high=mode #57564

mark108 mannequin opened this issue Nov 6, 2011 · 38 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@mark108
Copy link
Mannequin

mark108 mannequin commented Nov 6, 2011

BPO 13355
Nosy @rhettinger, @terryjreedy, @larryhastings, @asvetlov, @skrah, @serhiy-storchaka
Files
  • random_triangular.patch
  • issue_13355.patch
  • fix_triangular.diff: Catch ZeroDivisionError
  • 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:

    assignee = 'https://github.com/rhettinger'
    closed_at = <Date 2014-05-26.00:41:31.967>
    created_at = <Date 2011-11-06.10:56:56.195>
    labels = ['easy', 'type-bug', 'library']
    title = 'random.triangular error when low = high=mode'
    updated_at = <Date 2014-05-26.08:35:02.390>
    user = 'https://bugs.python.org/mark108'

    bugs.python.org fields:

    activity = <Date 2014-05-26.08:35:02.390>
    actor = 'serhiy.storchaka'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2014-05-26.00:41:31.967>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2011-11-06.10:56:56.195>
    creator = 'mark108'
    dependencies = []
    files = ['28975', '29709', '35333']
    hgrepos = []
    issue_num = 13355
    keywords = ['patch', 'easy']
    message_count = 38.0
    messages = ['147148', '147149', '147150', '147155', '147548', '147564', '181512', '181522', '181525', '181527', '181528', '181532', '181539', '181544', '181568', '181654', '186197', '186571', '186678', '186680', '186681', '186694', '186699', '186721', '186728', '186730', '187836', '187839', '207792', '207913', '209273', '219015', '219019', '219033', '219112', '219113', '219114', '219145']
    nosy_count = 9.0
    nosy_names = ['rhettinger', 'terry.reedy', 'larry', 'asvetlov', 'skrah', 'python-dev', 'mark108', 'serhiy.storchaka', 'Chaka_bum']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = 'needs patch'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue13355'
    versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

    @mark108
    Copy link
    Mannequin Author

    mark108 mannequin commented Nov 6, 2011

    When low and mode are the same in random.triangular it gives the following error:

    <type 'exceptions.ZeroDivisionError'>: float division
    args = ('float division',)
    message = 'float division'

    When high and mode are the same there is no problem.

    @mark108 mark108 mannequin added the extension-modules C modules in the Modules dir label Nov 6, 2011
    @mdickinson
    Copy link
    Member

    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 low and high, and that's what's causing the error.

    @mark108
    Copy link
    Mannequin Author

    mark108 mannequin commented Nov 6, 2011

    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.

    @mdickinson
    Copy link
    Member

    it would be convenient if low=high=mode returned low (or mode or high)

    Yes, I agree that random.triangular should degrade gracefully, in the same way that random.uniform does.

    @rhettinger rhettinger self-assigned this Nov 6, 2011
    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy terryjreedy added stdlib Python modules in the Lib dir and removed extension-modules C modules in the Modules dir labels Nov 13, 2011
    @terryjreedy terryjreedy changed the title random.triangular error when low = mode random.triangular error when low = high=mode Nov 13, 2011
    @terryjreedy terryjreedy added the type-bug An unexpected behavior, bug, or error label Nov 13, 2011
    @rhettinger
    Copy link
    Contributor

    I've got this one.

    @serhiy-storchaka
    Copy link
    Member

    Here is a patch.

    @mdickinson
    Copy link
    Member

    Here is a patch.

    Where? :-)

    @serhiy-storchaka
    Copy link
    Member

    Now it is here.

    @mdickinson
    Copy link
    Member

    Looks fine to me. Raymond: can this be applied?

    @mdickinson
    Copy link
    Member

    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.

    @serhiy-storchaka
    Copy link
    Member

    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?

    @mdickinson
    Copy link
    Member

    What if low == high != mode?

    Then we've got garbage in, garbage out; that case doesn't worry me.

    @serhiy-storchaka
    Copy link
    Member

    An exception is better than a garbage result. But I agree, triangular()
    currently is not protectet against a situation when mode is not in low--high
    range.

    @mdickinson
    Copy link
    Member

    An exception is better than a garbage result.

    Agreed. And ZeroDivisionError is the wrong exception, too---ValueError would be better. But I'm content that the current patch fixes the immediate issue.

    @rhettinger
    Copy link
    Contributor

    I'll look at the patch shortly. At first glance, it looks over-engineered to me.

    @Chakabum
    Copy link
    Mannequin

    Chakabum mannequin commented Apr 7, 2013

    Added validation of input data. Check whether low <= mode <= high. If low == high return low as a result.

    @asvetlov
    Copy link
    Contributor

    Test for the issue_13355.patch is incorrect, please fix.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 12, 2013

    New changeset c40e36a49033 by Andrew Svetlov in branch '3.3':
    Issue bpo-13355: Raise ValueError on random.triangular call with invalid params.
    http://hg.python.org/cpython/rev/c40e36a49033

    New changeset 613eb432b152 by Andrew Svetlov in branch 'default':
    Issue bpo-13355: Raise ValueError on random.triangular call with invalid params.
    http://hg.python.org/cpython/rev/613eb432b152

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 12, 2013

    New changeset 1062c66e9bdc by Andrew Svetlov in branch '2.7':
    Issue bpo-13355: Raise ValueError on random.triangular call with invalid params.
    http://hg.python.org/cpython/rev/1062c66e9bdc

    @asvetlov
    Copy link
    Contributor

    Pushed.
    Yuriy Senko, thanks for patch.
    But, please, be more accurate with tests for another one.

    @rhettinger
    Copy link
    Contributor

    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.

    @rhettinger rhettinger reopened this Apr 13, 2013
    @rhettinger
    Copy link
    Contributor

    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.

    @mdickinson
    Copy link
    Member

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 13, 2013

    New changeset e948154af406 by Andrew Svetlov in branch '3.3':
    Revert changes for bpo-13355 by request from Raymond Hettinger
    http://hg.python.org/cpython/rev/e948154af406

    New changeset 39bbbf5d7b01 by Andrew Svetlov in branch 'default':
    Revert changes for bpo-13355 by request from Raymond Hettinger
    http://hg.python.org/cpython/rev/39bbbf5d7b01

    New changeset 620c691c12c5 by Andrew Svetlov in branch '2.7':
    Revert changes for bpo-13355 by request from Raymond Hettinger
    http://hg.python.org/cpython/rev/620c691c12c5

    @asvetlov
    Copy link
    Contributor

    Reverted. Sorry.

    @mdickinson
    Copy link
    Member

    Raymond: have you had time to look at this yet?

    @rhettinger
    Copy link
    Contributor

    Soonish

    @serhiy-storchaka
    Copy link
    Member

    Raymond, could you please make a decision or delegate this issue to Mark, Terry, Andrew or me?

    @rhettinger
    Copy link
    Contributor

    [Serhiy]

    Raymond, could you please make a decision

    Yes, I will this week.

    @larryhastings
    Copy link
    Contributor

    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.

    @rhettinger
    Copy link
    Contributor

    Attaching patch

    @serhiy-storchaka
    Copy link
    Member

    So now triangular(10, 10, 20) will always return 10?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 24, 2014

    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

    @rhettinger
    Copy link
    Contributor

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 26, 2014

    New changeset 7ea6c8eb91e2 by Raymond Hettinger in branch '3.4':
    bpo-13355: Make random.triangular degrade gracefully when low == high.
    http://hg.python.org/cpython/rev/7ea6c8eb91e2

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 26, 2014

    New changeset 6dc5c4ba7544 by Raymond Hettinger in branch '2.7':
    bpo-13355: Make random.triangular degrade gracefully when low == high.
    http://hg.python.org/cpython/rev/6dc5c4ba7544

    @serhiy-storchaka
    Copy link
    Member

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants