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

Simplify the do_filtered_draw hook #2988

Merged
merged 3 commits into from May 30, 2021

Conversation

Zalathar
Copy link
Contributor

This is a small piece of cleanup that I've been meaning to do for a while.

Originally, the do_filtered_draw hook was added as a lightweight way for individual strategies to make filtering more efficient (specifically sampled_from in #1904), without having to fully override .filter with a custom strategy.

(At the time, no non-trivial strategy had a custom .filter implementation, so this was worthwhile.)

Over time, a few things changed:

All together, these developments mean that the original design of do_filtered_draw is now more of a liability, adding confusion for little gain.


This PR therefore greatly simplifies the design of do_filtered_draw by omitting the filter_strategy argument, and changing its contract to simply be “like do_draw, but if filtering fails then return a sentinel instead of throwing”.

Furthermore, most strategies now no longer have a do_filtered_draw method at all. Instead they have a _filter_for_filtered_draw method, which returns an object (currently always FilteredStrategy) that implements do_filtered_draw.

(Note that SampledFromStrategy still has a do_filtered_draw method, but currently it never actually gets called by UniqueListStrategy because of the UniqueFilteredListStrategy specialization. I've left it as-is for now, partly because it would work if needed, and partly to avoid having to merge that code back into SampledFromStrategy.do_draw for no real benefit.)

@Zalathar Zalathar added the internals Stuff that only Hypothesis devs should ever see label May 29, 2021
@Zalathar Zalathar force-pushed the do-filtered-draw branch 2 times, most recently from a1dbad9 to c69cad6 Compare May 29, 2021 07:03
This is documented in the implementation of `JustStrategy`, and there
are some other tests that happen to fail if it isn't true, but there was
previously no explicit test for this specific property.
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

This looks great - thanks Zalathar. I really appreciate all the cleanup work you've been doing lately 😍

@Zalathar Zalathar merged commit fb35be1 into HypothesisWorks:master May 30, 2021
@Zalathar Zalathar deleted the do-filtered-draw branch May 30, 2021 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internals Stuff that only Hypothesis devs should ever see
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants