Implement LazySequenceCopy.pop(i)
#3987
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tybug
force-pushed
the
lazy-sequence-copy-pop
branch
from
19a7ec3 to
20417ac
Compare
Zac-HD
approved these changes
May 13, 2024
Comment on lines
+83
to
+86
| # this would be more robust as a stateful test, where each rule is a list operation | ||
| # on (1) the canonical python list and (2) its LazySequenceCopy. We would assert | ||
| # that the return values and lists match after each rule, and the original list | ||
| # is unmodified. |
There was a problem hiding this comment.
I agree that this would be nice but is not merge-blocking important.
tybug
force-pushed
the
lazy-sequence-copy-pop
branch
from
f83b58e to
9110e34
Compare
jobh
reviewed
May 14, 2024
hypothesis-python/src/hypothesis/internal/conjecture/junkdrawer.py
Outdated
Show resolved
Hide resolved
jobh
approved these changes
May 14, 2024
| shrunk = minimal(st.sampled_from(LargeFlag), lambda f: bit_count(f.value) > 1) | ||
| # Ideal would be (bit0 | bit1), but: | ||
| # minimal(st.sets(st.sampled_from(range(10)), min_size=3)) == {0, 8, 9} # not {0, 1, 2} | ||
| assert shrunk == LargeFlag.bit0 | LargeFlag.bit63 # documents actual behaviour |
There was a problem hiding this comment.
Nice, I remember scratching my head over this!
There was a problem hiding this comment.
thanks to your comment here, it was the only reason I looked into this 🙂
|
Looks like we're no longer triggering the hypothesis/hypothesis-python/src/hypothesis/internal/conjecture/engine.py Lines 379 to 382 in 4e77eea |
tybug
force-pushed
the
lazy-sequence-copy-pop
branch
from
9541a82 to
9368da7
Compare
|
that coverage was hopefully fixed in #3991. I've rebased on master, let's see how it goes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
UniqueSampledListStrategyusesLazySequenceCopyto pop elements from the list. BecauseLazySequenceCopyonly implemented pop-from-end, we swapped the element we wanted to pop to the end before popping. But this leaves the list in a bad state for shrinking: ifi = 0, we just swapped the last element, which is the most complicated in shrinking order, to the front of the list, leaving it there for future passes. This means an ir tree of[0, 0, 0]indicating the first three elements of the sequence corresponds to{0, 8, 9}forsets(range(10)).The solution is properly implementing
.pop(i). I've applied this in other places we use the swap trick, though I'm less confident thanUniqueSampledListStrategythat this is not a behavioral change.