-
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
ENH implement plot_monthly_returns_timeseries in plotting.py to rende… #195
Conversation
…r monthly bars representing monthly returns of an algo
I want to add this plot as a default in tears.create_returns_tearsheet() as well. But might make sense to do that as a separate PR after this one is merged, along with after this one: #189 I looked at where it would make sense to add it to the tearsheet in tears.create_returns_tearsheet(), and it feels like it would be a bit of an intrusive modification given all the inter-connectedness of the plots in the figure/grid spec |
@twiecki let me know if you think we can merge this. It only involves additions to the code base, and no modifications to anything existing, so pretty low risk. Let me know if it looks good to you. |
|
||
# Generate month-year labels for the x-axis corresponding to | ||
# the returns plotted | ||
date_labels = [str(i[1]) + ' - ' + str(i[0]) for i in |
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.
What's wrong with the default labels of this plot? I suppose there's too many? How about just thinning those. The current approach feels a bit brittle.
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.
yep that's right, the defaults are super ugly because it prints the month/day/year of each rendered monthly bar and the plot becomes illegible if more than ~30 bars.
As well, the defaults include the last business day of the month in each label (which just adds to the cluttered visual because the last business day of each month jumps around, 29, 30, 31, etc). My opinion is that including the day in the label is not really proper when labeling a monthly value, as well.
Pandas does not have a monthly frequency unit so I could not up-sample the DateTimeIndex to make it more elegant (which would have been best).
Given the above, and my desire to have an easily interpreted plot, how would you suggest improved pruning of the labels? I actually adapted my implementation from an accepted StackOverflow response
Sent from iPhone
_____________________________
From: Thomas Wiecki notifications@github.com
Sent: Wednesday, November 4, 2015 4:10 AM
Subject: Re: [pyfolio] ENH implement plot_monthly_returns_timeseries in plotting.py to rende… (#195)
To: quantopian/pyfolio pyfolio@noreply.github.com
Cc: Justin Lent justinlent@gmail.com
In pyfolio/plotting.py: > + Returns> + -------> + ax : matplotlib.Axes> + The axes that were plotted on.> + """> +> + if ax is None:> + ax = plt.gca()> +> + monthly_ret_table = timeseries.aggregate_returns(returns, 'monthly')> + monthly_ret_table = monthly_ret_table.reset_index()> + monthly_ret_table.columns = ['year', 'month', 'returns']> +> + # Generate month-year labels for the x-axis corresponding to> + # the returns plotted> + date_labels = [str(i[1]) + ' - ' + str(i[0]) for i in
What's wrong with the default labels of this plot? I suppose there's too many? How about just thinning those. The current approach feels a bit brittle.
—
Reply to this email directly or view it on GitHub.
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.
@twiecki ...Actually, my bad, looks like Pandas does support a Monthly unit, under the concept of a Period. so timestamps can be converted to periods to drop the concept of a "day" in monthly data and have plots generated properly. I'll go ahead and re-implement... Ohhh Pandas, so convoluted sometimes...
Sent from iPhone
On Wed, Nov 4, 2015 at 4:10 AM -0800, "Thomas Wiecki" notifications@github.com wrote:
In pyfolio/plotting.py:
- Returns
- ax : matplotlib.Axes
The axes that were plotted on.
- """
- if ax is None:
ax = plt.gca()
- monthly_ret_table = timeseries.aggregate_returns(returns, 'monthly')
- monthly_ret_table = monthly_ret_table.reset_index()
- monthly_ret_table.columns = ['year', 'month', 'returns']
Generate month-year labels for the x-axis corresponding to
the returns plotted
- date_labels = [str(i[1]) + ' - ' + str(i[0]) for i in
What's wrong with the default labels of this plot? I suppose there's too many? How about just thinning those. The current approach feels a bit brittle.
—
Reply to this email directly or view it on GitHub.
@twiecki I re-implemented using PeriodIndex - much cleaner implementation for sure. As well rendering some yearly boundary dashed-lines for viewing convenience. |
@justinlent Looks great! |
ENH implement plot_monthly_returns_timeseries in plotting.py to rende…
…r monthly bars representing monthly returns of an algo