akumidv / tradingview-assistant-chrome-extension

An assistant for backtesting trading strategies and checking (showing) external signals in Tradingview implemented as a Chrome browser extension.
GNU General Public License v3.0
176 stars 63 forks source link

Export/Import pitfalls #96

Open Jason5480 opened 1 year ago

Jason5480 commented 1 year ago

Hello @akumidv. I have heard quite a lot lately about your plugin and I was really interested in the export/import functionality it provides so I took the chance and gave it a try. So, I created a small "proof of concept" example that fits my needs and can be found here TradingView_Assistant_Proof_of_Concept.txt. As you might have guessed I had an unpleasant surprise when I found that none of the exported settings was able to be imported correctly. However, I can see the potential of what you are doing here and this is why I spent the time filling out this report. To be more specific on what I did, and how you can reproduce my findings after creating the code I posted above (you can copy-paste it into the "Pine Editor" directly and add it to your chart). Then, I opened the Settings/Input dialog and I changed the "Moving Average Length"s and the "Moving Average Color"s for both Fast and Slow. Then I changed the alert messages I want to receive for Long and Short positions. Then I moved to the Strategy/Properties setting and I set the commission to a better reflect my case. last b ut not least I went to the Settings/Style and unchecked some other options. You can find screenshots of the exact setup here. exported_inputs exported_properties exported_style

After filling in all of my settings I pressed the export button and the file produced by the tool was the following. TVA PoC.csv

Then I pressed the "Reset settings" to get the default back. I pressed the import button and used the previously exported file I attached previously. The result can be shown in the screenshots below. imported_inputs imported_properties imported_style

At this point, I want to thank you for providing this tool to the community and I really appreciate this open-source contribution. I am a big fan of the concept but I am also javascript illiterate :D By inspecting the *.csv file I came to the following conclusions and I would ask for your help/opinion on that matter.

That's all I have for now! Let me know your thoughts on this.

Best, Jason

Edit: After creating the example I posted above I realized that I use time range filters very often in my scripts and usually I have two timestamps From and To. The pinescript code look like this:

` fromDateTime = input.time(defval = timestamp('01 Jan 2022 00:00'), title = 'From', group = 'Time Window')

toDateTime = input.time(defval = timestamp('01 Jan 2023 00:00'), title = 'To', group = 'Time Window') `

But those timestamps weren't exported either.

theupslidedown commented 1 year ago

Bump for interest in the issue and resolution to be able to use the tool to export more reliably. Thank you for the detailed information, Jason.

myn commented 1 year ago

Bump for interest. I use this extension daily and would love to see this outstanding issue resolved! Thank you Jason for documenting this.

akumidv commented 1 year ago

@Jason5480 Thank you for your testing and so detailed review. I'll check the problems that you described.

Jason5480 commented 1 year ago

@Jason5480 Thank you for your testing and so detailed review. I'll check the problems that you described.

@akumidv thank you for taking the time to look into this issue! I updated the pinescript code I posted above so it also includes the timestamps and you can easily reproduce all my findings. Being a software engineer (C++) myself I know that a good/detailed report is important to make the ticket findings easy to understand and solve. Especially in open-source projects where none is obligated to spend it's own time on something. I would make a pull request myself if I could, but unfortunately javascript is not my "strong point". Thank you for your time once again and if there are any questions please feel free to ask.

akumidv commented 1 year ago

@Jason5480 thank you for your detailed test for export/import parameters that also used for settings them during backtesting.

When I was testing, I found an error - the parameters setting cycle breaks early that all parameters are set if some of parameters have identical names. It is fixes in new 2.2 version

For the rest of the parameters, there are two groups of problems here:

PS also it doesn't save "Properties" and "Style" tabs. I'll add to list of future improvements.

Jason5480 commented 1 year ago

Thank you @akumidv for the blazing fast analysis of all of the findings! I am glad that you fixed the identical names issue. My guess was that in the previous version you used the title as a unique id of a associative container. But, you cannot assume that the title given by the pinecoder is unique, maybe a combination/hash of the title name, group name and the inline increment could be used to create this unique id as you mentioned above. I look forward to test the new version!

Now about the first group of the problems. To be honest I couldn't care less about the color :D I wrote it down just for completeness and you have a fair point that only parameters that affect the backtesting results should be saved here. However, this is not the case with the timestamps. In the pinescript example I use that to evaluate the strategy in different time windows and that has different backtest results. So it would be convenient to just load back my *csv and reproduce the same backtest results. Usually, I use the text fields for alert messages because it is easier to see them in the settings tab. The alerts might not be considered "functional" for the strategy itself but someone else could use those strings in a "functional way". What's' more setting up the alerts for a 'bot framework' e.g. 3Commas again and again might be time consuming. It would be very beneficial for the end user to configure the alert messages once. Now for saving "Properties" and "Style" tab is a 'nice to have' feature given that the strategy itself has meaningful defaults. On the other hand "Properties" settings might have a big impact in the backtest results of a strategy, so according to the "useful backtesting ideology" those should also be included maybe latter on. There is no rush in this. I appreciate your fast response on that matter. Thank you very much once again!

akumidv commented 1 year ago

I didn't solve the problem with identical names, only solve another problem when identical names present on parameters. About them and hidden names for parameters either I'm holding in mind as priority problem.

I've understood the functional needs and add this issue to list of improvements.

I'm just not sure about schedule to solve them. It will demand changes in structure of csv so both of this problem lead to changes in parameters model structure and changes everywere in code.

Jason5480 commented 1 year ago

I didn't solve the problem with identical names, only solve another problem when identical names present on parameters. About them and hidden names for parameters either I'm holding in mind as priority problem.

I've understood the functional needs and add this issue to list of improvements.

I'm just not sure about schedule to solve them. It will demand changes in structure of csv so both of this problem lead to changes in parameters model structure and changes everywere in code.

I see that this will be a breaking change. So, I do not have the expectation of exported settings with previous versions to work with the most recent one. However, in the new *.csv you could save the schema version and during import, if the schema version information is not there, or it is not compatible, you could write a "fail to import message" to the user. Being a software engineer myself I know that development takes time to do it "the right way" and also test it before an official release. So, take your time. You have a better understanding of the importance of the existing tickets and the priority you should give to each one of them.