Closed luca-s closed 6 years ago
Oh, I forgot. To properly compute the portfolio returns I needed to know the length of the periods. Currently they are just integers and it is not known the units of those integers. So I changed the period column names from a simple integer to a pd.Timedelta string representation (1D, 5H, etc)
I'll go through pep8 issues tomorrow.
Ok, finished with pep8 issues. This PR is ready
This looks great!
The PR is ready to be merged but I expect some issues with pyfolio as I haven't done extensive testing. Anyway I don't expect regressions with existing functionalities and I actually expect better compatibility with non-daily factors and event studies.
@twiecki I am going to merge this if you agree
Does this change existing behavior?
No, all scenarios that were working before are working now + the current behaviour does a better job in handling factors with gaps (when values are not computed every day, e.g. event study) and intraday factors.
The only user level visible change is the period column names that are now named "1D", "3D" (1 day, 3 days or "1H", 3H" for 1 hour, 3 hours, etc. ) instead of "1", "3".
OK, great. And users can now also have arbitrary periods, like hours.
I haven't reviewed the code carefully but I'm fine merging this as an experimental feature.
Actually, let's see if @richafrank has someone who could do a code review.
Actually arbitrary periods are possible even without this PR, for example a daily factor with 1 hours period or a weekly factor (computed on Monday for example) with a period of 3 days. With this kind of factors the results are computed correctly even though the turnover analysing is not so interesting, but the cumulative returns are wrong and this prevent the pyfolio analysis, this is why I fixed it in this PR and, as I was already fixing the code, I made sure the cumulative returns is correct with factors not computed at a fixed frequency, e.g. event study
@luca-s @twiecki this looks neat! I had a couple questions around the UX:
create_pyfolio_input
, I expected it to have positions and transactions as well. Since the function's output is the return stream, should we change its name to clarify that it's providing only returns, and not all of the other inputs that pyfolio consumes?You are right @abhijeetkalyan , the name is confusing a little. The reason is that I initially thought to return positions
and transactions
as well but I haven't managed to do that yet. I am actively working on this so my plan is to improve create_pyfolio_input
to make it returns positions
and transactions
in another PR. I hope to push that PR soon together with an example NB. If I manage to do that before we create a new tag I would avoid the warning, otherwise if we create a tag and release a new version of Alphalens before I finish working on this feature I will add a warning message. Sounds like a plan?
I added an example NB and also I made create_pyfolio_input
returns the benchmark returns too (as the factor universe mean returns).
I also added a warning in create_pyfolio_input
docstring:
WARNING: this API is still in experimental phase and input/output paramenters might change in the future
I believe this PR is now ready to be merged
@prsutherland thanks for reminding me about the walk through NB, I completely forgot about that. Regarding the default values on the period
parameters I am not sure... If I find a sensible default I would restore them.
The movement from integers to Timedelta strings for period was needed to carry information that otherwise would have been lost but some thorough testing is now required because this change could lead to some hidden and non-obvious bugs. Sometimes I think get_clean_factor_and_forward_returns should return an object where we can store all the information we want together with factor_data instead of trying to save the information we need inside the DataFrame column names...but for now let's keep it this way.
Unless there are other comments I am going to merge this PR soon. I only need to finish updating the tear_sheet_walk_through NB.
Looks great, @luca-s. Thanks!
Issue #225
Added performance.create_pyfolio_input, which create input suitable for pyfolio. Intended use:
Also, I greatly improved the function that computes the cumulative returns as that is the base on which the pyfolio integration feature is built on. Mainly I removed the legacy assumptions which required the factor to be computed at specific frequency (daily, weekly etc) and also the periods had to be multiple of this frequency (2 days, 4 days etc). The function is now able to properly compute returns when there are gaps in the factor data or when we analyze an intraday factor.