-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Combining built-in indicators with TA-lib indicators #837
Comments
Yes in theory there should not be any difference, could you please share specific backtests? |
Sure thing! poloniex | USDT | XRP [RSI] Adding that single line in init() changes the simulated profit from 100% to -81% |
Is anyone else able to confirm this behavior? |
I will test asap! I can tell you that as soon as you add a talib indicator the execution flow changes slightly (since these indicators are now calculated in async). But if that results in different outputs it is a bug. |
Mike, I know you're super busy and already put in a ridiculous amount of work for this project (thank you!). I was sorta hoping someone else could give it a try, as it should only take a few mins (assuming the import is done) :) Where's everyone else at? lol |
Ok, I have not experienced this. Can you upload your two strats and config files somewhere and I will test it. Edit; interesting I never put the indicator add line anywhere but init. I think if you put it in check you are effectively not getting the indicators time component. So an ema20 will always be an ema1 in check. I could be wrong but that's what I guess. |
Here's my files: FYI this was backtested via the UI.
In config.js:
In Kane2.js, un-comment line 37 to see the results change: Thanks for your help! |
@thegamecat any luck? |
Nope sorry. Will try tomorrow! |
No worries! I haven't been using Gekko since I ran into this, because if something so basic doesn't work as expected, I have no confidence in the rest of the app lol. Hopefully I'm just doing something wrong. |
I've run your code with ADX disabled and enabled. ADX commented out: ADX enabled: 2017-07-19 11:07:56 (INFO): (PROFIT REPORT) start time: 2017-01-07 19:25:00 |
Well shit :( @thegamecat can you please try using this: poloniex | USDT | XRP |
Oh wait, I misread what you posted! Yours is also showing different numbers simply by including the ADX indicator but not using it. Thanks @thegamecat for confirming. @askmike I think we found a bug ... |
I have just tested with an extra talib.adx and I see the same results, this is very strange. I'll stop what I was working on with gekko and fix this asap first. |
Thanks Mike! I wish I was smart enough to help debug! |
Talib does expand the dataset for sure. I've written indicators that don't use talib but need the richer dataset. |
Okay I've figured out the problem: Gekko is written in javascript. Whenever you do any kind of IO (any type of "call" to something outside your javascript code) you do that call and specify what you want to happen once it is done (at a later stage). If you use talib indicators in your strat gekko will do a call to talib to calculate the indicators, once this is done Gekko will give control to your strat (including the indicator results). However by the time TAlib gave Gekko the results of an indicator for a given candle, the non talib indicators might be from the "next candle". This bug is a race condition which only affects strats that have:
|
Interesting ... good catch! |
I just commited a hotfix, can you verify whether it works for you? |
Could you double check if the roundtrips (table below the chart) are the same? Because sharpe ratio is calculated based on these. |
This is most likely a similar bug that manifested itself in a very different part of the codebase, a part that has changed a lot (esp for #737). I'll look into this asap. |
This is odd. If I pull your updated basetradingmethod file into my gekko I get a segmentation fault on the db when running a backtest through cli. Have reverted back to previous version now. |
That's really strange, the baseTradingMethod does not interact with any
database..
…On Fri, Jul 21, 2017 at 12:08 PM, thegamecat ***@***.***> wrote:
This is odd. If I pull your updated basetradingmethod file into my gekko I
get a segmentation fault on the db when running a backtest through cli.
Have reverted back to previous version now.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#837 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA7MDzPq-TcO77eu-OTRKbeyzs_iMtnXks5sQIafgaJpZM4OSJp8>
.
--
PGP key at keybase.io/mikevanrossum
<https://keybase.io/mikevanrossum/key.asc>
|
I know, I am utterly confused. I've tried different dbs and trading methods and get the same result. I need to run the full latest code base - I've been waiting for your major update before doing it though :) |
Ah yes, sorry about the delays. Extremely busy at the moment, loads of stuff for work (different clients), a lot going on personally. In these cases I unfortunately have to defer working on Gekko.. |
HAHA I didn't mean it that way - just that I am waiting :) |
@thegamecat I'm using 'feature/ui-trader' branch with only difference being the updated baseTradingMethod.js file. No segmentation fault noticed. Running CentOS. |
Ta, I'll try that when I can. |
Quick update on this - I've installed and tested the feature/ui-trader branch and it's working fine with my strat which includes standard and talib indicators. I ran master and feature/ui side by side and got slightly improved trades with the feature/ui. Edit: re-running this, I think it's down to a different slippage value. Will see. What I've concluded is that change to the baseTradingMethod clearly causes a problem with the master codebase for me. Looking forward to the merge with develop. |
Another edit....I've just realised the feature/ui branch doesn't have the base trading fix in it.... I've added the base trading fix into both Master and Feature/ui and theyr're both running now. We will see how long for - it's a lot slower is my initial finding. |
Feature/ui branch just failed: <--- Last few GCs ---> [3865:0x26d5a50] 677113 ms: Mark-sweep 1400.0 (1439.4) -> 1400.0 (1439.4) MB, 882.2 / 0.1 ms (+ 1.1 ms in 1 steps since start of marking, biggest step 1.1 ms) allocation failure GC in old space requested <--- JS stacktrace ---> ==== JS stack trace ========================================= Security context: 0x29f6fe6c0d39 FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory |
And now Master branch fails: <--- Last few GCs ---> [3871:0x2e19a40] 784283 ms: Mark-sweep 1399.9 (1437.2) -> 1399.8 (1437.2) MB, 1657.6 / 0.0 ms allocation failure GC in old space requested <--- JS stacktrace ---> ==== JS stack trace ========================================= Security context: 0x3feff7bc0d39 FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory |
You don't have enough memory, this is not an issue with gekko. |
Memory in what sense? This is a 32GB machine. I'm running this on Develop now, will see what happens, but it's definitely trading differently which is expected. |
Same thing on Develop branch: [5372:0x2c15a50] 794084 ms: Mark-sweep 1399.3 (1436.4) -> 1399.3 (1436.4) MB, 1712.7 / 0.0 ms allocation failure GC in old space requested <--- JS stacktrace ---> ==== JS stack trace ========================================= Security context: 0x4063c1c0d39 FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory |
So, the question is - why does this happen with the basetrading change? It's not happening with the previous version. I don't think this is a closed issue as the fix is causing a problem. |
@thegamecat this is definitely a closed issue since:
is now fixed. If you think the fix introduced a new issue, please open a new issue and provide more information (out of heap is definitely memory related, so please post what you are trying to do as well as Gekko output (as opposed to v8 stack traces). |
Ok, will do. |
Looks like this has reappeared in some form, issue #1689 |
I realize this is probably a dumb question, but I'm not sure why, if I copy-paste the /strategies/RSI.js code into a new custom strategy .js file, then simply add:
Within the init() method, the backtesting results change drastically. Don't I have to actually use the ADX indicator results within the check() method for the backtest to change? Why, simply by adding the indicator do the results change?
The text was updated successfully, but these errors were encountered: