BlackArbsCEO / Adv_Fin_ML_Exercises

Experimental solutions to selected exercises from the book [Advances in Financial Machine Learning by Marcos Lopez De Prado]
MIT License
1.7k stars 633 forks source link

Error in getTEvents #7

Closed tmorgan4 closed 5 years ago

tmorgan4 commented 6 years ago

It seems this error goes all the way back to the code provided in the book. Really appreciate you making your examples available.

sNeg is off by a minus sign causing it to be reset to 0 every loop. sNeg<-h is never executed. Events are only being triggered on the positive side (sPos>h). Subtracting diff.loc[i] seems to be the easiest fix but you might prefer a different way.

tmorgan4 commented 6 years ago

Upon closer inspection I'm incorrect about where the error is. The issue is actually the .abs() in diff = np.log(gRaw).diff().dropna().abs(). Taking the absolute value means only positive values are accumulated and sNeg never accumulates any value while sPos is always counting up regardless of whether the difference is positive or negative.

While plotting the values I also spotted a separate issue. The daily volatility mean is being used as an input: tEvents = getTEvents(close,h=dailyVol.mean()). This is a definite issue with 'peeping into the future' as the .mean() would never be known until the very end. Maybe we should consider using a rolling window with some number of previous daily volatilities (and not the current day).

BlackArbsCEO commented 6 years ago

Hi @tmorgan4,

Thanks for the insightful feedback and correction. Before I merge the pull request, I think it would be very instructive for others (including me), if you could demonstrate the differences in output between the old version and the fixed version using a notebook; especially the effect it has on the labeling exercises. If/when you provide that I'd be happy to merge your pull request including the notebook demo.

Also good catch on the leakage. A rolling window could work, I'll have to think on it a little bit more when I have some time.

tmorgan4 commented 6 years ago

I'd be happy to put together some documentation on the changes. Give me a day or so and I'll get it posted.

BlackArbsCEO commented 6 years ago

@tmorgan4 sure let me know when you're ready, can't wait to see the notebook breakdown.

tmorgan4 commented 6 years ago

@BlackArbsCEO I apologize for the delay on getting the notebooks added. I spent a few days trying to get everything working on my work PC and eventually gave up. It works much smoother on my mac but it means I need to work from home. I should be able to get it put together this week.

lionelyoung commented 6 years ago

@tmorgan4 if your work PC happens to be windows, I found that Cell 23 can be modified to utilize the following platform check to run in single-threaded mode:

import platform
if platform.system() == "Windows":
    cpus = 1
else:
    cpus = cpu_count() - 1
BlackArbsCEO commented 5 years ago

I fixed the getTEvents typo in 977a92b83ded86171a73bea6e1bd73d5c48f2d3b. Apologies for the delay.