-
Notifications
You must be signed in to change notification settings - Fork 616
Refactor the reference section of the docs #4311
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
Conversation
@@ -1,19 +1,8 @@ | |||
==================== | |||
Reproducing failures | |||
==================== |
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.
My heading ordering is single =, single -, single ~ (and if you need to go beyond that you're probably doomed anyway). I try to avoid double headers for anything, but I think I might be the odd one out here, because e.g. the python rst guide recommends double headers for chapters. In defense of single headers, the python docs have way more structure than we do!
ab_c = (a + b) + c | ||
a_bc = a + (b + c) | ||
difference = abs(ab_c - a_bc) | ||
target(difference) # Without this, the test almost always passes |
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.
As fas as I can tell, target
makes little difference here. Around 40% discover rate with or without it.
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.
same..I'm not going to follow up on this right now, but I've added it as a TODO to #4309
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! A comment for that time (ignore for this PR) is that target
likely doesn't work well here because difference
is constant for large regions of the input space.
We might want to comment that the target should be fairly smooth, not discrete-valued, and not added willy-nilly as it may actually reduce bug-finding power with the default example budget (as indicated below).
For background, the optimiser seems to get more stuck than I'd expect in these flat regions. E.g.,
@given(st.floats())
@settings(database=None, phases=[Phase.generate, Phase.target])
def test_target(f):
target(f > 1e5)
print(f)
doesn't vary f
very much inside the True
region. Fair enough, but perhaps worth noting.
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.
This is great. I think target
used to be better at this on the bytestring since a continuous in bytes could easily lead to discontinuous change in the output, which is less likely on the typed choice sequence. We might have to reintroduce a simulated annealing-style approach for each of the 5 primitive types.
Quick thoughts again:
|
yeah it's a fair point, I'm on board with one big page for the api reference. It'll be a bit awkward until we move the more tutorial-style things out from the api reference, but still an improvement on the status quo. |
so I know we said just one or two reference pages, but I was working through this and I think there's a valid distinction between "api/coding reference" and "'meta' reference", like observability or the pytest plugin, which I've kept in separate pages for the moment. They don't feel right to me in an api reference page, because they don't have an in-library method exposed by Hypothesis. |
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 think we will eventually want to move this page somewhere better ("how to write type hints for your strategies"?), or remove it entirely, but top level is fine during the transition
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.
Agree on both counts. We could start it out as e.g./how-to/type-annotate-strategies
so that we can keep the URL longer term; I think it can still be top-level in the TOC regardless of location.
I've added a test to check that all exported names are documented, as recommended. This turned up a bug in the current docs where |
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.
More scattered thoughts, and appreciation for the ongoing work 🙏
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.
Agree on both counts. We could start it out as e.g./how-to/type-annotate-strategies
so that we can keep the URL longer term; I think it can still be top-level in the TOC regardless of location.
.. automodule:: xps | ||
:members: | ||
from_dtype, |
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.
Oooh... does your excellent new test check that this is complete? It's not an importable module, exactly...
Strategy objects tell Hypothesis what types of inputs to generate. For instance, passing ``st.lists(st.integers(), min_size=1)`` to |@given| tells Hypothesis to generate lists of integers with at least one element. | ||
|
||
Strategies can be combined using :ref:`combinator strategies <combinators>`, or modified using |strategy.filter|, |strategy.map|, or |strategy.flatmap|. |
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.
Strategy objects tell Hypothesis what types of inputs to generate. For instance, passing ``st.lists(st.integers(), min_size=1)`` to |@given| tells Hypothesis to generate lists of integers with at least one element. | |
Strategies can be combined using :ref:`combinator strategies <combinators>`, or modified using |strategy.filter|, |strategy.map|, or |strategy.flatmap|. | |
Strategies are the way we describe values for |@given| to generate. For instance, ``st.lists(st.integers(), min_size=1)`` tells Hypothesis to generate lists of integers with at least one element. | |
This reference page lists all of Hypothesis' first-party functions which return a strategy; there are also many provided by <third-party libraries>(xref), and you can define your own too. Note that people often say "strategy" when they mean "function returning a strategy"; it's usually clear from context which they meant. | |
Strategies can be passed to many strategy functions as arguments, combined using :ref:`combinator strategies <combinators>`, and modified using |strategy.filter|, |strategy.map|, or |strategy.flatmap|. |
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.
Strategies can be passed to many strategy functions as arguments
I get the distinction you're making, and I think it's worth making, but this is a confusing sentence to parse imo. Bite the technical incorrectness and say "strategies can be passed to other strategies as arguments"?
I've applied a variant of this, but happy to tweak further
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 do think we need to call out both the distinction and the common sloppy usage in this explanatory header, but don't care exactly how we do that.
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 think we're done with this part - obviously there are plenty of followups, but I feel good about shipping this as a single cohesive change for our users 😁
e6cfb7a
to
eb7aeb9
Compare
Still a large pr but there's no sane way to split most of this without causing more work for me, thanks to all the inter-references. I could split the redirect extension off is probably the biggest thing
There's a minor amount of added or removed content here, but most of it is moving around existing stuff into their own page