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

Fixed a mistake in the pairs trading lecture #303

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

quantopiancal
Copy link
Contributor

An error was pointed out by community member Emilio via helpscout:

Hi, I found a mistake in this lecture. In the moving average section at the end, there is this piece of code:

# Calculate rolling beta coefficient
rolling_beta = pd.ols(y=S1, x=S2, window_type='rolling', window=30)
spread = S2 - rolling_beta.beta['x'] * S1

I noticed that in the rolling beta calculation y=S1 and x=S2. However, when calculating the spread S2 now becomes the dependent variable Y and S1 is the independent variable X
Thanks, Emilio

Rene confirmed it was a mistake and offered a fix via slack:
image

That fix is reflected in the code change.

@@ -978,7 +978,7 @@
"source": [
Copy link
Member

Choose a reason for hiding this comment

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

👍 on the fix to the source. It looks like the cell output is also stored in the notebook. Should we re-run the cell and save the new output?

@quantopiancal
Copy link
Contributor Author

@richafrank Jamie asked me to test the change for truthiness. Will re-run to change the cell output and test for truthiness.

@richafrank
Copy link
Member

Nice. Ok, new request for you: Would it be possible to run this in an environment where we aren't generating all the FutureWarnings or saving them in the output for everyone?

@quantopiancal
Copy link
Contributor Author

Hey Rich, I did my best to cut down on those error messages - there is one error left.

I did my best to alter the code while keeping the results exactly the same, but don't have the know how to convert pd.ols to whatever the statsmodels equivalent is.

I would have just run the code in a notebook with the original pandas version (0.17.1), but the notebook requires using get_pricing(), which is unavailable locally.

@richafrank
Copy link
Member

Ah, I didn't realize the warnings were from 0.18 - I had assumed you were running this locally with a newer version! I wouldn't worry about the remaining warnings then. Definitely stick with what's on the platform.

Thanks for updating to Series.rolling.

  1. Does std_30 = spread.rolling(window=30).std() need center=False, like the others?
  2. Since this will be copied by lots of folks, could you add a space before the center=False args that you added, so we conform to PEP8?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants