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

Various core and test changes #3835

Merged
merged 21 commits into from
Jan 11, 2024

Conversation

tybug
Copy link
Member

@tybug tybug commented Jan 10, 2024

Split from #3818. I could split this further if desired, with all the draw_bits changes in a single PR.

Changes:

There were some test changes where the extra boolean draw from > 24 bit integers caused the test to fail.

if bits > 24 and self.draw_boolean(
7 / 8, forced=None if forced is None else False
):

I mostly remedied this by reducing the bit size down to 20 for these tests. It does make me wonder if we're doing the appropriate thing by biasing the distribution in this way at the ir level, rather than at the strategy level (integers()).

@tybug tybug requested a review from Zac-HD as a code owner January 10, 2024 20:36
@tybug tybug mentioned this pull request Jan 10, 2024
5 tasks
@tybug
Copy link
Member Author

tybug commented Jan 10, 2024

Looks like test_can_learn_to_normalize_the_unnormalized became flaky due to this (e.g.). Will look into it.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 11, 2024

I mostly remedied this by reducing the bit size down to 20 for these tests. It does make me wonder if we're doing the appropriate thing by biasing the distribution in this way at the ir level, rather than at the strategy level (integers()).

Hmm, good question! IIRC we used 2**24 as a special value because it ensured that unicode characters were chosen without (additional) bias. I'd probably leave it alone for now, but we should do an audit of this kind of heuristic once we're using the IR everywhere - there are probably a bunch we'll be able to clean up then.


General thoughts on this PR: looks good to me. Two approaches you can take to this kind of pulled-out stage are to (1) defer anything that we can't merge ~immediately to a future PR, or (2) keep working on it here until it passes to make future PRs easier. I generally favor a bit of (2) followed by (1) if I'm feeling stuck or just slowed down; it's basically the same question as pulling out part in the first place 😁

@tybug
Copy link
Member Author

tybug commented Jan 11, 2024

that approach makes sense 👍.

In this case I didn't get stuck for too long on test_can_learn_to_normalize_the_unnormalized. It was a rare case where a test is failing because we did too good of a job 😄. dfa.normalize only adds a new normalization if the shrinking differs within 100 calls. With draw_bits, the shrinking differed within that time frame. After using the IR, the shrinking was usually consistent (hence the flakiness). Which means we were doing a better job shrinking to a normalized example with the IR than without!

The solution? Make the property even harder to shrink to a normalized value.

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.

🎉

Comment on lines -137 to +140
while data.draw_bits(2) == 3:
# TODO this test fails with data.draw_boolean(0.25). Does the hill
# climbing optimizer just not like the bit representation of boolean
# draws, or do we have a deeper bug here?
while data.draw_integer(0, 3) == 3:
Copy link
Member

Choose a reason for hiding this comment

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

I'm trusting you to track this; I'd probably link to this code from a comment on the IR issue after merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

left a task in the pr description 👍 (less likely for me to lose it than a comment, and github doesn't easily let me leave reviews on unrelated files.)

@Zac-HD Zac-HD merged commit 00d19ca into HypothesisWorks:master Jan 11, 2024
47 checks passed
@tybug tybug deleted the various-core-touchups branch January 11, 2024 22:55
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.

None yet

2 participants