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

Proposal for a more flexible bounded floats strategy #1622

Closed
rob-smallshire opened this issue Oct 5, 2018 · 10 comments
Closed

Proposal for a more flexible bounded floats strategy #1622

rob-smallshire opened this issue Oct 5, 2018 · 10 comments
Assignees
Labels
question not sure it's a bug? questions welcome

Comments

@rob-smallshire
Copy link
Contributor

rob-smallshire commented Oct 5, 2018

Motivation

In many of the applications where I benefit from using Hypothesis, I find myself needing a strategy for generating floats drawn from half-open intervals. The existing hypothesis.strategies.floats strategy draws from a closed interval, and though it's straightforward enough to exclude one or both of the limits using assume() in the body of the test, this makes the test somewhat harder to quickly comprehend, and the assume() is easily forgotten.

Proposal

This is a proposal for a strategy capable for generating floats drawn from half-open or open internals, or modification of the existing floats strategy to support the convenient specification of such intervals.

Interface

I've been using a strategy called bounded_floats which instead of min_value and max_value to specify the interval, uses inclusive_min_value xor exclusive_min_value, and inclusive_max_value xor exclusive_max_value arguments. I accept these are somewhat wordy, but they clearly express the intent. In addition, the allow_nan, allow_infinity and width arguments are also supported.

Typical use would be (from real code):

@given(temperature=bounded_floats(
    inclusive_min_value=-273.15,
    exclusive_max_value=-46.0
))
def test_for_design_temperatures_below_minus_46_the_material_of_construction_is_ss_316(temperature):
    # test body

Implementation

I've been using the following simple wrapper for the existing floats strategy:

@composite
def bounded_floats(
        draw,
        inclusive_min_value=None,
        exclusive_min_value=None,
        inclusive_max_value=None,
        exclusive_max_value=None,
        allow_nan=None,
        allow_infinity=None,
        width=64):
    if (inclusive_min_value is not None) and (exclusive_min_value is not None):
        raise TypeError(
            "Only one of inclusive_min_value and exclusive_min_value can be specified. "
            "inclusive_min_value = {}, exclusive_min_value = {}".format(
                inclusive_min_value,
                exclusive_min_value
            ))
    if (inclusive_max_value is not None) and (exclusive_max_value is not None):
        raise TypeError(
            "Only one of inclusive_max_value and exclusive_max_value can be specified. "
            "inclusive_max_value = {}, exclusive_max_value = {}".format(
                inclusive_max_value,
                exclusive_max_value
            ))
    min_value = inclusive_min_value if (inclusive_min_value is not None) else exclusive_min_value
    max_value = inclusive_max_value if (inclusive_max_value is not None) else exclusive_max_value
    f = draw(floats(
        min_value=min_value,
        max_value=max_value,
        allow_nan=allow_nan,
        allow_infinity=allow_infinity,
        width=width))
    if exclusive_min_value is not None:
        assume(f != exclusive_min_value)
    if exclusive_max_value is not None:
        assume(f != exclusive_max_value)
    return f

Alternatives

I've thought about, and experimented with, some alternative designs, which are worth considering:

  • Separate functions for open_floats(), right_half_open_floats(), left_half_open_floats(), closed_floats(), where closed_floats() is just an alias for the existing floats(). I found it difficult to remember whether the left or right bound was included or excluded.

  • I tried open_min_value and closed_min_value, etc for the argument names, but it turns out that many programmers aren't familiar with the open/closed interval terminology. In any case, inclusive and exclusive have the pleasing aesthetic property of having the same number of characters, as well as being widely understood.

  • I haven't tried this, but one idea would be extend the existing floats strategy to support at least exclusive_min_value and exclusive_max_value in addition to the existing min_value and max_value arguments, which would be retained for backwards compatibility. It's also possible to support inclusive_min_value and inclusive_max_value aliases for the existing arguments in the spirit of allowing clearer interval specifications.

  • All this becomes unnecessary if you can easily generate the floating point numbers immediately inside the discrete closed interval which corresponds to the discrete (half-)open interval. In C and C++ we have the nextafter and nexttoward functions which return the adjacent representable floating point numbers. Unfortunately, these aren't exposed in Python's math module. Using functions like them, it should be possible to using the existing floats strategy like this:

    @given(temperature=floats(
        min_value=-273.15,
        max_value=next_toward(-46.0, -273.15)  # exclusive bound
    ))
    def test_for_design_temperatures_below_minus_46_the_material_of_construction_is_ss_316(temperature):
        # test body

    though this could get messy when trying to account for the width argument to floats.


If there's sufficient interest in this proposal, I'll submit a pull request.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 5, 2018

Happily, we have implemented next_up and next down in hypothesis.internal.floats, and it already handles various widths! This is technically private/unstable API, but we're going to need it for as long as we e.g. accept integers as bounds for floats so you'd be pretty safe 😄

If you also need further floats strategies or arguments, I'd suggest starting with an experimental package to shake out the API - once you have some more experience with it we'll be able to judge whether the extra power justifies the risk of confusing new users (and the pain of migrating compatibility layers).

TLDR, I'm sympathetic but in this case conservative!

@Zac-HD Zac-HD added the question not sure it's a bug? questions welcome label Oct 5, 2018
@DRMacIver
Copy link
Member

How would we feel about include_min and include_max arguments? I think this is a common enough use case that it's worth making it easy for users.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 6, 2018

How would we feel about include_min and include_max arguments? I think this is a common enough use case that it's worth making it easy for users.

We already have a lot of logic to deal with tricky endpoints, so it would not just be a matter of incrementing or decrementing the float bound.

Take the example of min_value=Fraction('1/3'), include_min=True. This number cannot be exactly represented as a float, so we (currently) calculate the closest float above in order to respect the property that no example may be less than min_value. But in doing so, we would violate the intuitive property that min_value can be generated if include_min == True! Making it an error to pass a non-float min_value for inclusive bounds would be backwards-incompatible, so we can't do that either.

On the other hand, floats(x, y).filter(lambda n: n != x) is a pretty trivial way to exclude one or both endpoints. Perhaps (cc @rob-smallshire) adding an explicit hint to the floats() docs about using .filter to exclude bounds would resolve the issue? It even works for other strategies with bounds!

In short, I think the .filter solution is acceptable though more docs would be useful, while adding an argument would make the semantics quite horribly complicated.

@rob-smallshire
Copy link
Contributor Author

Using filter works. Using assume works. Both express the how to generate the required values, rather than declaring what I want to be generated. For me, this is an important distinction in the context of specifying program behaviour through tests.

Hypothesis gives us some powerful tools to adapt strategies in map, filter and flatmap, allowing us to define ad hoc strategies as fairly complex functional programs, inline. My use of Hypothesis has evolved to a position where I almost never use these facilities without encapsulating them in a named composite strategy, because when I'm reading a test I don't want to be figuring out these mini-programs. I want them to obviously have no mistakes, not to have no obvious mistakes. These miniature inline functional programs also lie beyond the reach of testing, which bothers me a bit.

So in practice, though I could use filter, or assume directly, I won't because I already have an encapsulated strategy which allows me to declare what I want, rather than having to explain every time how to generate it. Of course, I completely respect your desire to manage the growth and complexity of the core Hypothesis API, and understand the software engineering trade-offs in doing so.

@DRMacIver
Copy link
Member

On the other hand, floats(x, y).filter(lambda n: n != x) is a pretty trivial way to exclude one or both endpoints.

If there's a pretty trivial way to exclude one or both of the endpoints, what's the difficulty with us implementing the feature in that pretty trivial way?

@DRMacIver
Copy link
Member

Also if the main objection is just that it seems like include_min=True should mean that we can generate the endpoint, how about about making the arguments exclude_min and exclude_max?

@rob-smallshire
Copy link
Contributor Author

I have a preference for exclude_min=False and exclude_max=False as defaults over include_min=True and include_max=True. I have difficulty expressing why though. I think it's because adding new options which default to "off" feels better than adding new options which default to "on". In light of that, you should probably discount my opinion and think about which options new users would prefer to encounter.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 6, 2018

If there's a pretty trivial way to exclude one or both of the endpoints, what's the difficulty with us implementing the feature in that pretty trivial way? ... how about about making the arguments exclude_min and exclude_max?

My problem isn't implementation - it would be 4-10 LoC, and we already have piles of nasty internal logic for floats - it's whether users can easily form a correct mental model of how floats() actually works.

exclude_min and exclude_max are better, but if I knew nothing about Hypothesis seeing exclude_max=False in the signature of floats() would make me highly suspicious and if I knew less about floating point I'd think that the endpoints could be generated by default (and again, this is not true if you use integers or fractions or decimals). And at risk of a slippery slope argument: why would we add these arguments to floats() but not all the other strategies which accept min_value and max_value?

Basically I just don't think this option is a good fit for Hypothesis core, because if you need it you can just define a helper in your own project:

def floats(min_value=None, max_value=None, *, exclude_min=False, exclude_min=True, **kw):
    s = st.floats(min_value, max_value, **kw)
    if exclude_min is not None:
        s = s.filter(lambda x: x != min_value)
    if exclude_max is not None:
        s = s.filter(lambda x: x != max_value)
    return s

Our contributing guide mentions a preference for composable primitives - IMO the impact on other (especially new) users is too large to justify the convenience of a feature that is easy enough to compose downstream.

@DRMacIver
Copy link
Member

Basically I just don't think this option is a good fit for Hypothesis core, because if you need it you can just define a helper in your own project:

I think this is a bad argument. A lot of functionality in Hypothesis core is stuff you could easily add helpers for to your own project, and that's a good thing. The aggregate of many trivial usability features is not a trivial usability improvement, but a large one.

why would we add these arguments to floats() but not all the other strategies which accept min_value and max_value?

TBH adding them to the other strategies which accept min_value and max_value seems pretty reasonable to me. Maybe what we need is a generic Interval abstraction?

again, this is not true if you use integers or fractions or decimals

I'm kinda with Rob, I think we should deprecate the possibility of passing min_value and max_value that are not exactly representable as floats, which makes this problem go away.

IMO the impact on other (especially new) users is too large to justify the convenience of a feature that is easy enough to compose downstream.

I don't think you've convincingly argued a large impact on users at all TBH. The proposed feature has straightforward semantics, is a fairly common use case, and the absolute worst case scenario is that in a scenario that is already highly misleading the user might be slightly more mislead if they don't read the documentation.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 6, 2018

I've said my piece, and I'm happy to go with the consensus for adding them 😄

I'm kinda with Rob, I think we should deprecate the possibility of passing min_value and max_value that are not exactly representable as floats, which makes this problem go away.

Agreed! I've opened #1625 as a good-first-issue for someone to do this in Hacktoberfest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question not sure it's a bug? questions welcome
Projects
None yet
Development

No branches or pull requests

3 participants