-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: added positions computation in 'performance.create_pyfolio_input' #250
Conversation
I noticed th pyfolio Exposure plot is empty. I am not sure if it is a pyfolio bug or if the data misses something. The positions are computed as percentage and the cash too, is this correct? e.g.
|
2f8a03d
to
459e4ee
Compare
If anybody has a better name for the new API (or the new internal functions) please let me know because I am not so happy about them but I couldn't think of anything better. |
I've just realized that positions must be in dollars. Only |
I think that |
I like the idea of moving I thought about a wrapper too but I discarded the idea because it doesn't add anything useful and also we would have to keep updating the alphalens API to reflect the changes that happens on pyfolio. More importantly I don't like the idea of hiding pyfolio calls as it is interesting for the user to understand what functionality is called so that they can customize the calls for their needs (there are so many parameters in pyfolio tears functions). Let's see if calling pyfolio becomes more difficult in the future but as long as it is as simple as now we can keep it the way it is. What do you think? |
That makes sense to me. It's a case of doing the whole performance attribution in two lines vs. one line, which I think is okay to leave as two for now. |
60a9145
to
ada4fff
Compare
@twiecki it is ready to be reviewed. Positions are now compute as dollar amount instead of percentage. Actually Pyfolio results are identical to before so I wonder if we could stick to percentage positions as I like them more and also the users wouldn't be forced to provide an initial capital in |
ada4fff
to
e9d9309
Compare
d33335c
to
e9d9309
Compare
e9d9309
to
b718b81
Compare
But shouldn't the logic just select the top and bottom n stocks? Seems like it's weighting by alpha signal. |
So the point of confusion is that we are asking for a portfolio that has these characteristics: The problem is that the code demeans the factor values and go long on the positive ones and short on the negative ones and then it computes equal weights. This is the cause of the confusion. I need to think again about this behavior and makes sure it doesn't end up with this kind of inconsistencies. Thank you for spotting this out, I love when the bugs are found right away :) |
Not sure I understand yet what the problem really is. Shouldn't the 1 and 5 quantile have roughly the same number of stocks despite what weighting is used? |
Yes and I will fix that, it has to work as you say. I believe I was looking at the equal weighting from the wrong point of view. I used the factor values to decide what assets should be long and what short and then I compute the equal weighting. I actually have to use the quantile information to decide what asset should be in the short positions and what in the long ones. |
One other idea for the future would be the ability to supply a custom weighting function. E.g. could see the case for equal weight, alpha weighted, inv vol etc. |
b718b81
to
8d95794
Compare
alphalens/performance.py
Outdated
|
||
from pandas.tseries.offsets import Day |
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.
Should that be BDay
?
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.
I was in doubt. That is used only to save an error condition that should never happen (freq not set in the factor_data) so it's just a "safety belt" default value. I can switch that to BDay though
55a9eb0
to
93e2872
Compare
The issue with the long/short weights should be fixed now and NB updated too. By the way, the change I made to the weights computation is that factor values above the median become long positions, while factor values below the median become short positions. The previous behaviour was very similar except I used the mean instead of the median, that's why the extremely huge negative factor value for ES made it to be the only short position |
d1bd17b
to
63f7ea6
Compare
63f7ea6
to
ad0980e
Compare
I also found the reason of the Exposure plot being blank, it turned out to be a pyfolio bug. |
That makes sense. Ideally I think that would be configurable as well, e.g.: |
Also, seems like sub-sampling doesn't quite do what we want: ideally we wouldn't exit the positions but hold them for the whole week. I suppose one would need to have the same signal for all days in the week to achieve that. |
For now it's possible to choose which quantile to use, eventually we can add the percentile option if the quantile configuration is not flexible enough.
The portfolio is holding the positions for 1 day because the code calls I believe that rebalancing every 5 days is not the best way of trading a 5 days signal. A better way to do that would be to trade 1/5 th of the portfolio every subsequent day and rebalance each 1/5th portfolio every 5 days. This would result in the same transaction cost, but the slippage impact would be 1/5th, the portfolio capacity would be 5 times bigger, the volatility of the portfolio would be lower, the factor would be traded every single day making it more statistically robust and independent of the starting day. I can still modify the NB to show the usage of 5 days period traded every Monday, except I need a good excuse to show that. |
If the quantile is already used, where does the median (or mean) value come in when building the portfolio? The holding period question is tricky indeed. Although I think trading a 5-day signal every 5 days is a pretty simple method to go with as a default. |
I am not sure I understand your question. This is how I implemented it: the option
Ok then, I'll update the NB. |
ad0980e
to
f2c7a95
Compare
Oh I see. So first you select whatever quantiles the user specified, e.g. |
Exactly but please let me know if you have a better idea. Eventually I'd like to add your idea of a custom weighting function though, so the users can do what they like |
OK, that makes sense. An alternative would be to require the user to specify NB looks great too. I will try to review or find someone to review the code in more detail. @richafrank Do you know of someone who could help review this new feature? |
Making the long/short quantiles explicit would be nicer but then we would still need the |
Ping @richafrank. |
Thanks for the ping. Sorry I lost track of this. Will find someone! |
@@ -379,6 +386,11 @@ def cumulative_returns(returns, period, freq=None): | |||
if freq is None: | |||
freq = returns.index.freq | |||
|
|||
if freq is None: |
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.
You might consider combining this with the if statement on line 387.
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.
You mean moving that "if" inside the previous one? uhmm, I don't like too many levels of indentation
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.
Just that both if statements are evaluating freq is None
so the statements within each if statement can be combined into one block.
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.
That would change the logic as freq is changed inside the first "if" statement
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.
Ah! Good point!
if freq is None: | ||
freq = weights.index.freq | ||
|
||
if freq is None: |
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.
Same comment about this if statement and line 528
group = group.copy() | ||
|
||
if _equal_weight: | ||
if _demeaned: | ||
# top assets positive weights, bottom ones negative |
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.
Using median
instead of mean
is slightly confusing. It might need further clarification. Maybe update the docstring to "...If demeaned is True then the factor universe will be split in two equal sized
groups,..."?
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.
I like "equal sized groups". I will update that, thanks.
Plus it is now possible to select the 'period' to be used in benchmark computation
f2c7a95
to
b99d89b
Compare
Waiting on @prsutherland's sign-off before merging. |
@prsutherland any more comments on this PR? |
OK, I think this went through some solid review. Going to merge this -- really cool feature @luca-s! |
'performance.create_pyfolio_input' now computes positions too. Also it is now possible to select the 'period' to be used in benchmark computation and for factor returns/positions is now possible to select equal weighing instead of factor weighing.