API: run Alphalens with returns instead of prices (utils.get_clean_factor)

[HereticSK]

For issue #253 refactor compute_forward_returns add get_clean_factor API refactor get_clean_factor_and_forward_returns as compose of compute_forward_returns and get_clean_factor

[HereticSK]

For tests, I don't know much about writing test cases. So currently I just slightly modified the test_compute_forward_returns just enough for the new compute_forward_return to pass. It may need further discussion to design test cases for other functions.

For docs, I just slightly modified the original doc string to suit the new API, and made reference to get_clean_factor_and_forward_returns for full explanation. Anything else should be documented?

[luca-s]

Thank you @HereticSK, this PR is pretty neat. I will review it soon

[luca-s]

Regarding the tests the problem is that there weren't in the first place :) More seriously get_clean_factor_and_forward_returns is partially tested inside test_tears.py (the code tests the API don't raise exceptions). We should really add some tests in test_utils.py, especially now that we have two functions. Let me know if you plan to add those tests, I can guide you through the details. Otherwise we leave this for another PR.

[luca-s]

@twiecki I don't see Travis CI tests started automatically for this PR. Has something changed recently?

[HereticSK]

I'm afraid I don't have enough time to finish writing the tests. For the moment let's just finish the refactoring first. I'd be happy to help with the testing PR when I have more time.

[luca-s]

So, Travis CI runs fine but I don't see the link in the PR page anymore. Can anybody see that?

[luca-s]

@HereticSK When you are working again on this PR, would you mind rebasing on master? There are few commits that are relevant to this PR. Thank you!

[HereticSK]

@luca-s Sorry for the late response. Will find time to finish this PR this week.

[twiecki]

@HereticSK Is this ready for a new round of reviews?

[HereticSK]

@twiecki Yes, this commit improved the documentation, and rebased onto the latest master, as @luca-s suggests. I should have left a comment here to notice you guys, sorry for that :)

[luca-s]

@HereticSK is it possible that you haven't pushed your changes after rebasing? it seems HereticSK:get_clean_factor branch is still behind Alphalens master and there are conflicts

[twiecki]

@HereticSK let us know if you need help with that, excited to merge this.

[HereticSK]

Here are the git command I ran

git checkout master
git pull upstream master
git checkout get_clean_factor
git rebase -i master

and then I got

error: could not apply ebb6664... refactor compute_forward_returns When you have resolved this problem, run "git rebase --continue". If you prefer to skip this patch, run "git rebase --skip" instead. To check out the original branch and stop rebasing, run "git rebase --abort". Could not apply ebb66645f921ffa1941c2ceef313ae0d46489f55... refactor compute_forward_returns add get_clean_factor refactor get_clean_factor_and_forward_returns

It seems to point to commit ebb6664, but I don't know which lines are causing the problem.

[luca-s]

@HereticSK after you get the error messages, if you run git status command you'll be able to see the files with conflicts. If you open those files you'll see the conflicting lines marked with <<<<<<<, =======, >>>>>>> (the conflict markers). Delete the conflict markers and fix the conflicting code then add the conflicting files to the staging area (git add). Finally complete the rebase with "git rebase --continue"

[HereticSK]

@luca-s I seem to have fixed the git rebase issue. Let me know if there is anything needs to be improved.

I don't know why Travis checks failed. It seems to be a problem caused by things related to event-study?

[luca-s]

@HereticSK the Travis CI errors are due to a new matplotlib version. Alphalens needs to be updated for that, but it is not a problem of your PR.

[luca-s]

@HereticSK if you rebase on master again, the Travis CI issue should be fixed

[twiecki]

Looks great @HereticSK, can you add this to the release-notes (with authorship)?

[luca-s]

I am going to merge this now

@twiecki just to be sure, Do you mean we can now create the new release v0.3.0 + release-notes ?

[twiecki]

@luca-s great. And yes, that sounds perfect.

[luca-s]

Thank you @HereticSK !

[twiecki]

This is a great contribution @HereticSK!