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

Overlay OOS datapoints in boxplot #306

Merged
merged 4 commits into from
Jun 6, 2016
Merged

Overlay OOS datapoints in boxplot #306

merged 4 commits into from
Jun 6, 2016

Conversation

analicia
Copy link
Contributor

@analicia analicia commented Jun 2, 2016

Point markers are diamonds instead of crosses.

@twiecki
Copy link
Contributor

twiecki commented Jun 4, 2016

Looking at this code I think it's time to refactor it. Specifically,

    df_weekly = timeseries.aggregate_returns(returns, 'weekly')
    df_monthly = timeseries.aggregate_returns(returns, 'monthly')

Should happen inside the plotting function and only returns be passed in. The slicing is a bit confusing and this refactor could remedy this.

@analicia
Copy link
Contributor Author

analicia commented Jun 6, 2016

@twiecki df_weekly and df_monthly are now only defined by the plotting functions that require them.

@twiecki
Copy link
Contributor

twiecki commented Jun 6, 2016

LGTM, thanks!

@twiecki twiecki merged commit 764bcb9 into master Jun 6, 2016
@twiecki twiecki deleted the pyfolio_issue_183 branch June 6, 2016 20:41
@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

image

@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

@ahgnaw just noticed that we should probably add a legend as well explaining the red dots.

@analicia
Copy link
Contributor Author

analicia commented Jun 7, 2016

image

Unfortunately matplotlib adds the line through the marker in the legend. What do you think of the text? Alternative: "Out-of-sample returns"

@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

import matplotlib as mpl
mpl.rcParams['legend.handlelength'] = 0

Perhaps using that instead in a rc_context: http://matplotlib.org/api/matplotlib_configuration_api.html#matplotlib.rc_context

Also, the monthly color red is a bit hard to read with the red markers.

@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

I like out-of-sample data.

@analicia
Copy link
Contributor Author

analicia commented Jun 7, 2016

image

I'll try out a few different colors. The daily, weekly, and monthly markers could each be different colors.

@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

I think we'd rather switch the red color of the last bar.

@analicia
Copy link
Contributor Author

analicia commented Jun 7, 2016

image

image

here's yellow or green. I have a slight preference for the green, but either works.

@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

Sorry, I meant the actual fill color of the barplot, not the points.

@analicia
Copy link
Contributor Author

analicia commented Jun 7, 2016

Ah ok. I think this last. It's changed to a yellow that is consistent with the color palette. Thoughts?

image

@twiecki
Copy link
Contributor

twiecki commented Jun 7, 2016

Perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants