Skip to content

Conversation

@tobi-lipede-oodle
Copy link
Contributor

@tobi-lipede-oodle tobi-lipede-oodle commented Mar 1, 2021

Change Summary

This enables Hypothesis to generate a ConstrainedFloat with the multiple_of argument enabled.

Related issue number

#2443

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@samuelcolvin
Copy link
Member

is there any issue for this?

@tobi-lipede-oodle
Copy link
Contributor Author

Sorry! I'll make that now

@tobi-lipede-oodle tobi-lipede-oodle changed the title Adds option for generating a constrained float with multiple_of argument for hypothesis plugin Allow for generation of a constrained float with multiple_of argument for hypothesis plugin Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #2442 (661cc31) into master (f9fe4aa) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2442   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5084      5095   +11     
  Branches      1042      1048    +6     
=========================================
+ Hits          5084      5095   +11     
Impacted Files Coverage Δ
pydantic/_hypothesis_plugin.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9fe4aa...661cc31. Read the comment docs.

@PrettyWood
Copy link
Collaborator

PrettyWood commented Mar 2, 2021

@samuelcolvin I didn't add the v1.8.1 label since nothing is broken per se but it would probably make sense to add this PR in v1.8.1. Being able to use multiple_of only with int and not float with hypothesis is weird

@samuelcolvin samuelcolvin added this to the v1.8.1 milestone Mar 2, 2021
@samuelcolvin
Copy link
Member

may as well include this in v1.8.1

@samuelcolvin samuelcolvin merged commit 429b439 into pydantic:master Mar 2, 2021
@samuelcolvin
Copy link
Member

thanks so much.

@tobi-lipede-oodle tobi-lipede-oodle deleted the hypothesis-float-multiple branch March 2, 2021 19:00
if exclude_max:
max_value = max_value - 1

return st.integers(min_value, max_value).map(lambda x: x * cls.multiple_of)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strategy may generate integers in the case of multiple_of is an integer. I suggest using st.floats instead.

if max_value is not None:
assert max_value >= cls.multiple_of, 'Cannot build model with max value smaller than multiple of'
max_value = math.floor(max_value / cls.multiple_of)
if exclude_max:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to use the built-in exclude_max argument to st.floats instead of subtracting, as it reduces the possible range of generated values more than needed.

if exclude_min:
min_value = min_value + 1
if max_value is not None:
assert max_value >= cls.multiple_of, 'Cannot build model with max value smaller than multiple of'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion restricts the generation more than needed. For example, max_value=-2.0 and multiple_of=-1.0 will fail this assertion, but if the plugin generates -3.0, it satisfies the constraints.

Generally, I suggest taking a look at multipleOf implementation in hypothesis-jsonschema (there are multiple places in the linked file) as it handles all these constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants