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

Perf attrib tearsheets #441

Merged

Conversation

vikram-narayan
Copy link
Contributor

add perf attrib tearsheet
perf_attrib_tearsheet

@twiecki
Copy link
Contributor

twiecki commented Sep 12, 2017

This looks great. I would drop the histogram alpha plot (probably can delete it alltogether for now), and remove the stackplot from the exposures, as we discussed, and replace it with a line-plot for now.

@joshpayne
Copy link

+1 to replacing the stackplot with a line plot

@vikram-narayan
Copy link
Contributor Author

vikram-narayan commented Sep 12, 2017

made those changes, tearsheet now looks like this:

perf_attrib_tearsheet

@vikram-narayan
Copy link
Contributor Author

@yankees714 could you take a look when you get a chance?

@twiecki
Copy link
Contributor

twiecki commented Sep 13, 2017

I'm fine to merge after engineering review.

@yankees714 yankees714 self-requested a review September 13, 2017 13:10
Copy link
Contributor

@yankees714 yankees714 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had some minor suggestions for you.

"""
Plot each factor's contribution to performance.

Parameters
----------
exposures : pd.DataFrame
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this was just unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

colors=COLORS
)
for s in factors_and_specific:
ax.plot(factors_and_specific[s])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, can we use something like field or col here instead of s?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

labels=exposures.columns,
colors=COLORS)
for s in exposures:
ax.plot(exposures[s])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

pyfolio/utils.py Outdated
@@ -622,3 +623,9 @@ def set_legend_location(ax):

ax.legend(frameon=True, framealpha=0.5, loc='upper left',
bbox_to_anchor=(1.05, 1))

color_cycle = cycler('color', COLORS)
ax.set_prop_cycle(color_cycle)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we might be able to do this in one line, without importing cycler? Like:

ax.set_prop_cycle('color', COLORS)

https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.set_prop_cycle.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

positions,
factor_returns,
factor_loadings,
pos_in_dollars=pos_in_dollars
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing comma here will make future diffs cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -612,7 +613,7 @@ def get_symbol_rets(symbol, start=None, end=None):
end=end)


def set_legend_location(ax):
def set_legend_location(ax, autofmt_xdate=True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever pass autofmt_xdate=False?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't as of now, I wanted to pass it as an option in case we want to handle formatting elsewhere in future plots

@vikram-narayan
Copy link
Contributor Author

responded to @yankees714's feedback, will squash commits and merge once tests pass

vikram-narayan added 3 commits September 13, 2017 16:02
- pass all perf attrib data to plot_returns
- use line plot for factor contrib
- format x-axis dates on plots
@vikram-narayan vikram-narayan merged commit 4633cdf into quantopian:master Sep 13, 2017
@vikram-narayan vikram-narayan deleted the perf_attrib_tearsheets branch September 13, 2017 21:29
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.

4 participants