-
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
Add fix for #742 #743
Add fix for #742 #743
Conversation
Add sum_signed_rolling_deltas function and proper NumPy vectorization. Some TMO indicators in TV consider the rolling window to be exclusive of the current day, while others consider it inclusive. Since this series is used to compute the moving averages and momentums for the TMO, there would be a plot mismatch against TV unless we dealt with both cases. Added doctests for both cases, noting that since the series must be cast to float type, fp precision representation issues might arise, which makes the Pandas Series equality tests unsuitable. We use np.allclose so that we can deal with both the fp precision issues if they arise, and with the necessary NaN padding as well.
0749177
to
12c56da
Compare
Great! I'll merge it in this week. Also I downloaded BATS-SPY-D-TV-TMO.csv
Yeah nearly every indicator can have numerous variations these days. Naturally, we want to be accurate to one variant. If people do not like it, they can fork and adjust as needed. KJ |
Thank you. I assume that's TV data? That would be useful. I noticed volume is missing, but for the current case that is not an issue. Give me a few days to test this thoroughly. Thank you. |
No rush. Yup. It is TV Data. I stripped the volume since it was not necessary for this. Thank you 😎 |
@twopirllc apologies for the delay, but i can only take on this next week. |
No worries. |
Hi @luisbarrancos, Hope you are doing well and having a wonderful holiday! I believe something in your fix is breaking tmo's resultant DataFrame DateTime index causing the test ✔️ When I exclude tmo in that test, the test passes as expected. ❌ However, I receive the following when tmo is included: Do you have any ideas on how to resolve this issue? Out of curiosity, what OS are you developing on? Are you able to run the tests in the Makefile? If you are, for this case, you should only need to run I am in no rush do to the holidays and all. Thanks, |
Pardon my ignorance and forgetfulness, but upon further testing, the vwap output is correct for that test since it incrementally adds rows to the test DataFrame starting with zero rows. So that is to be expected and normal. 🤦🏼♂️ Additionally, I found out why All in all, its fixed and will be pushed soon. Enjoy the holidays! |
@twopirllc my sincere apologies for the late reply, the holidays did get in the way. |
No worries. Hope your holidays went well. 😎
Absolutely. All taken care of. 👍🏼 Wasn't urgent and I was able to find a fix quickly. Appreciate all the help. 😎 |
This fixes #742, adding a function to compute the sum of the signed deltas of the current day close and preceding days open, for a given window. Doctests are included, but it should be mentioned that since the series is padded with NaNs to match the input series lengths, Pandas Series equality tests aren't suitable, due to the integer to float promotion and the need to deal with NaNs, so we have rely on
np.allclose
function with precision limits and NaN dealing.Now the results match the TV indicators, but here again, some mentions are needed about the profusion of TMO indicators and implementations.
This implemention here is incorrectly computing the TMO since it's considering the current closing price and the preceding days opening prices as well as the current day opening price.
The remaining implementations in TV differ in how they consider the rolling window, if its length is exclusive of the current day, or inclusive. As such a new parameter
exclusive_window: bool = True
and its default, was added.This reflects the behaviour of what i think is the correctly implemented TMO indicator here. It should consider the current day close and the preceding days open prices for a lookback period of given length, hence exclusive rolling window.
The remaining TV implementations are variants of the TMO where they all consider the rolling window inclusive, and as such, they consider the$n-1$ days for the open prices.
The variants in question are this, this, this and this.
You should take the rolling window into consideration if you aim to replicate these TV charts, however bear in mind that it'll be very difficult to match them exactly due to not having the same market data as TV, and I suspect, the way the MAs are initialized in TV. Also have in mind that some of the TV variants use different initial periods for the MAs.