-
Notifications
You must be signed in to change notification settings - Fork 13
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
Clarify shrinking #13
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 8d86bf3.
There's some kind of weird edge case we're hitting now sometimes with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I've left some comments on things I think need improving or where I wasn't sure about the change, but I appreciate the work!
# Attempt to replace | ||
bin_search_down(0, self.result[i], lambda v: replace({i: v})) | ||
i -= 1 | ||
self.result = shrink_lower(self.result, consider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigning result
here is dodgy. It's not necessary (because calling the interestingness test automatically updates result
) and can only introduce bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't considered that nearly enough. It makes factoring out the shrinking passes in the first place rather hairy: we're relying on a side-effect in the InterestingnessTest
that gets passed in to actually update the result. Which is not to say it's bad, but it's not what I had in mind when I factored out the passes. I'll need to think about it a little bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are ways to shrink without side effects, see elm-microthesis. But it would need a larger refactor of the Python code than you perhaps intended.
self.result = shrink_redistribute(self.result, consider) | ||
|
||
|
||
def shrink_remove(current: array[int], is_interesting: InterestTest) -> array[int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these external functions instead of methods on the runner?
|
||
|
||
def shrink_remove(current: array[int], is_interesting: InterestTest) -> array[int]: | ||
# Try removing chunks, starting from the end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if some of these comments were extracted into docstrings if these are to be extracted into their own functions.
@@ -82,16 +82,15 @@ | |||
S = TypeVar("S", covariant=True) | |||
U = TypeVar("U") # Invariant | |||
|
|||
InterestTest = Callable[[array], bool] # Really array[int] -> bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer InterestingnessTest
Also what's with the comment here? Why can't this be an array[int]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The builtin array
type is not subscriptable, and so the language can't handle array[int]
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, weird. I wonder if this is a Python version thing? https://github.com/python/typeshed/blob/main/stdlib/array.pyi#L21 makes it look like array should be able to handle subscripting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed works when running on 3.12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.11:
minithesis.py:85: in <module>
InterestTest = Callable[[array[int]], bool] # Really array[int] -> bool
E TypeError: type 'array.array' is not subscriptable
This PR mainly extracts shrinking passes into their own functions, and tries to write these functions to be a bit more legible.