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

Strategies deduplication does not working for st.none as expected in OneOfStrategy.element_strategies #2327

Closed
Stranger6667 opened this issue Jan 17, 2020 · 2 comments · Fixed by #2330
Assignees
Labels
legibility make errors helpful and Hypothesis grokable question not sure it's a bug? questions welcome

Comments

@Stranger6667
Copy link
Collaborator

Hello!

Some time ago I created an issue - #2087
and recently got back to the investigations regarding the cause of the observed behavior. A took this case:

one_of(none(), none(), booleans())

That was generating 85 examples, while there are only possible 3 cases (None, True, False).

I found this code in OneOfStrategy that should remove duplicates (I assume):

pruned = []
seen = set()
for s in strategies:
    if s is self:
        continue
    if s in seen:
        continue
    seen.add(s)
    pruned.append(s)

But if s in seen doesn't work for st.none():

In [3]: seen = set()                                                                                                                                                                                                                                                                                                    

In [4]: seen.add(st.none())                                                                                                                                                                                                                                                                                             

In [5]: seen.add(st.none())                                                                                                                                                                                                                                                                                             

In [6]: seen                                                                                                                                                                                                                                                                                                            
Out[6]: {none(), none()}

And for just (which is used in none). I assume that "just" strategy instances can be deduplicated at least in some cases (for just(None) for example). What do you think? Could you, please, share some information about this behavior?

@Zac-HD Zac-HD added legibility make errors helpful and Hypothesis grokable question not sure it's a bug? questions welcome labels Jan 19, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Jan 19, 2020

See #2291, especially:

master...Zac-HD:hashable-strategies shows a current draft... which doesn't quite work. Hashing really doesn't play nicely with lazy instantiation and instances which are distinguished by mutable attributes... and we have those everywhere. So it's easy to make it work a little, but intractable to make it worthwhile.

Given that this is a minor performance issue but doesn't affect correctness at all, I'm inclined to leave things as they are 😕

(and for the 84 examples, see #2030/#2185/#2290 I think - TLDR nothing to worry about, it's an artifact of a bit-level representation on non-power-of-two numbers of alternatives that only really manifests on small examples)

I think it would be reasonable to add __hash__ methods to our strategies for hashable scalars, but I still don't think making all strategies hashable is worth the trouble. I should add a comment block explaining this to the OneOfStrategy implementation, though...

@Zac-HD Zac-HD self-assigned this Jan 19, 2020
@Stranger6667
Copy link
Collaborator Author

Aha, thank you for the insights! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable question not sure it's a bug? questions welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants