-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Multistrike cones #310
Multistrike cones #310
Conversation
Self-assigning, discussed with @twiecki |
|
||
Returns | ||
------- | ||
ax : matplotlib.Axes |
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 confused me a bit, because it feels like we're returning both ax
and fig
, whereas what we're actually doing is returning axes if the caller passed them in, and if not, we're returning a new figure. Could we find a way to make that clearer in the docstring?
@ahgnaw can we add a screenshot of what the multiple cones look like to the PR description? |
|
||
aggregate_returns.plot(ax=axes, | ||
lw=3., | ||
color='k', |
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 a little(very, tbh!) nitpicky, but can we say 'black
instead of k
here? It's a little more obvious what's going on with the explicit name instead of the shorthand.
Looks great! Just the one comment, then merge when travis is happy |
e258b40
to
33db3a0
Compare
Number of standard devations to use in the boundaries of | ||
the cone. If multiple values are passed, cone bounds will | ||
be generated for each value. | ||
random_seed : int | ||
Seed for the pseudorandom number generator used by the pandas | ||
sample method. | ||
num_strikes : int | ||
Upper limit for number of cones drawn. Can be anything from 0 to 3. |
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.
Minor nit on the docstring here - if num_strikes is 0, then we'll still have one cone drawn(even though we say in the docstring that the upper limit is 0). What do you think of rephrasing to something like:
num_strikes : int
Number of times to re-draw a cone if we fall outside the bounds of the previous one. Can be any int from 0 to 3.
33db3a0
to
4ed5724
Compare
4ed5724
to
fa62a68
Compare
fa62a68
to
20d22f9
Compare
Awesome work @ahgnaw and @abhijeetkalyan! |
New plot draws a green cone of expectation around the out-of-sample returns. When returns drop outside of a cone, a new cone is redrawn. Up to 4 cones are drawn, green, yellow, orange, and red.