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

ENH: add plot_risk_exposures #433

Merged
merged 3 commits into from
Sep 6, 2017

Conversation

vikram-narayan
Copy link
Contributor

@vikram-narayan vikram-narayan commented Aug 29, 2017

  1. plot risk factor exposures:
    risk factor exposures

  2. add flag pos_in_dollars to perf_attrib indicating whether positions are in dollars or not (for performance attribution does not work correctly when percentages are passed in #439)

@@ -225,7 +237,8 @@ def plot_factor_contribution_to_perf(exposures, perf_attrib_data, ax=None):
perf_attrib_data.index,
[perf_attrib_data[s] for s in chain(perf_attrib_data.iloc[:, :-3],
['specific_returns'])],
labels=list(perf_attrib_data.iloc[:, :-3]) + ['specific returns']
labels=list(perf_attrib_data.iloc[:, :-3]) + ['specific returns'],
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the last three dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the last three columns are total_returns, common_returns, and specific_returns, and common returns are accounted for by each of the individual factor returns

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe perf_attrib_data.drop(['total_returns', ...], axis='columns') then to make the intent more clear?

momentum reversal
dt
2017-01-01 -0.238655 0.077123
2017-01-02 0.821872 1.520515
Copy link
Contributor

Choose a reason for hiding this comment

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

document ax

if pos_in_dollars:
# convert holdings to percentages, and convert positions to long format
positions = get_percent_alloc(positions)

# remove cash after normalizing positions
del positions['cash']
Copy link
Contributor

@eigenfoo eigenfoo Sep 2, 2017

Choose a reason for hiding this comment

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

Perhaps positions.drop('cash', axis='columns') instead?

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

def set_legend_location(ax):
"""
Put legend in right of plot instead of overlapping with the it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo instead of overlapping with the it.

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

@@ -161,6 +180,15 @@ def show_perf_attrib_stats(returns, positions, factor_returns,
def plot_returns(returns, specific_returns, common_returns, ax=None):
"""
Plot total, specific, and common returns.

Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Also document returns, specific_returns and common_returns

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

@twiecki
Copy link
Contributor

twiecki commented Sep 4, 2017

See feedback from @eigenfoo but looks good otherwise.

@vikram-narayan
Copy link
Contributor Author

@yankees714 mind taking a look when you can?

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.

LGTM

'#b380ff', '#e0e6ac', '#a253a6', '#418020', '#ff409f',
'#ffa940', '#83ff40', '#3d58f2', '#e3ace6', '#d9a86c',
'#2db391'
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to put this in pyfolio.utils?

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, I'll move it there

# convert holdings to percentages, and convert positions to long format
positions = get_percent_alloc(positions)
if pos_in_dollars:
# convert holdings to percentages, and convert positions to long format
Copy link
Contributor

Choose a reason for hiding this comment

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

What does 'convert positions to long format' mean here?

Copy link
Contributor Author

@vikram-narayan vikram-narayan Sep 6, 2017

Choose a reason for hiding this comment

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

that part of the comment applies to the line positions = positions.stack(), it means change it from wide format:

                AAPL       TLT       XOM
2017-01-01  0.333333  0.568627  0.098039
2017-01-02  0.188034  0.658120  0.153846
2017-01-03 -0.263158  0.473684  0.526316

to long format (everything in one column with a multi index)

2017-01-01  AAPL    0.333333
            TLT     0.568627
            XOM     0.098039
2017-01-02  AAPL    0.188034
            TLT     0.658120
            XOM     0.153846
2017-01-03  AAPL   -0.263158
            TLT     0.473684
            XOM     0.526316

I'll change that comment to be in the right place

@vikram-narayan
Copy link
Contributor Author

I'll squash commits, and then merge

@vikram-narayan
Copy link
Contributor Author

once tests pass, I'll merge

@vikram-narayan vikram-narayan merged commit 79d7b16 into quantopian:master Sep 6, 2017
@vikram-narayan vikram-narayan deleted the more_plots branch September 6, 2017 20:34
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