MontrealTradingGroup / freqtrade

Simple High Frequency Trading Bot for crypto currencies
GNU General Public License v3.0
70 stars 61 forks source link

questions about sharpe ratio #2

Open xmatthias opened 5 years ago

xmatthias commented 5 years ago

This repo uses a wildly outdated version of freqtrade (more than 1000 commits/improvements behind) - and i think users should be warned about this fact.

I would appreciate if you could contribute your improvements back upstream instead of maintaining a outdated Version of freqtrade, which might lead to people loosing money and blaming it on us since it contains bugs which have long been fixed.

ilyasst commented 5 years ago

Hello Mathias,

We agree with your remarks. The repo keeps an outdated version because there is a class that relies on this repo. The freqtrade development team is doing an amazing job, but it is moving way too fast for us to keep updating the class each time a flag or command we present in the class changes or is removed.

During the class, we clearly explain this, and we clearly show the original freqtrade repository. The idea is that this repo will allow someone who is neither familiar with Linux/Terminal, nor with automated trading to get started quickly by following the class and getting the exact same results as the ones shown in the videos (we also provide the exact same VM as the one used during the class). Once a bit more comfortable, updating freqtrade is something we definitely recommend. This is the best solution we currently found, we are open to suggestions to improve this system as it is our first time doing a class this way.

The modifications brought to the code in this fork are mainly related to changing the loss function (minimizing the sharp ratio instead of the previous loss function). The purpose of the class made using this repository is mainly to teach an automated trading methodology and to provide Linux/Python basics. If people have bugs, they should have the exact same bugs we had. We hope that all users who take the class gain the knowledge necessary to then install the latest version of freqtrade themselves and then manage to use it efficiently by relying only on the freqtrade documentation. And then make some money, maybe :)

Thank you for the hard work, ilyass

xmatthias commented 5 years ago

Side note for the rest of this comment - so maybe some of these questions have been answered in the course: i did not take your class - and i don't have a udemy account IIRC.

If i may have a reccomendation then - maybe add your changes to a seperate branch (probably by using git cherry-pick - from looking at your current develop branch). You can then switch your fork to default to this new branch, which should (hopefully) avoid confusion and make it clearer what was added (and it should also simplify if you ever decide to upgrade to this new version).

also, avoid rebasing once code is used by others - that's really a pain to update if a user want's to EVER want to upgrade a checked-out branch, and requires some knowledge of git to do properly (beeing the user who did not rebase ...).

The only changes you've done as far as i can see is adding the sharpe-ratio - which is great.

Maybe we can get to a common version sooner or later (once the below is implemented, maybe - but i understand that upgrading the course to keep up is not easy) - and we can create a version (git tag) - so you can install exactly that version by using git checkout TAG_FOR_COURSE. This should make it easier for users to update to the uptodate thing, since it'll only be a git checkout develop away.

Another question: Are you optimizing sharpe for the riskiest strategy intentionally?

Someone copy/pasted your sharpe implementation to our comments (which made me aware of this repo). While trying to add this as optional loss-function (--loss sharpe) to hyperopt, i came across a few questions i'm not sure how to answer (i'm no expert in sharpe - but the way i understood it is that you want to optimize on the tangent, not the maximum number??)

https://github.com/freqtrade/freqtrade/issues/1907#issuecomment-511557113

ps: hope you don't mind us using both that implementation, as well as the idea to have sharpe ratio for hyperopt optimization.

ilyasst commented 5 years ago

@xmatthias Thank you for the suggestion ! It looks like a very good solution and I can understand the massive advantage of keeping the repository easily "upgradable". I will definitely do what you suggested for future projects if we need a repository. Give us a few days to look into it and we're going to try to do it for this repository first 👍

Regarding the Sharp ratio questions, I am going to let @MTG-Mohsen answer those, he is the trader after all :)

Thanks again for the hard work !

PS: No ot at all ! Actually quite the opposite :) We're super happy that you guys are considering the Sharp Ratio as an alternative loss function 😄

MTG-Mohsen commented 5 years ago

Hey @xmatthias,

I'm responsible for adding the sharp ratio in. I didn't really understand the loss function that was there so decided to work with what i knew. In all the firms that i worked at we used the sharp ratio to optimize any backtests that we did (some people used sortino ratio, but sharp ratio is the main one we used).

The reason is that sharp ratio takes into account risk and reward and nothing else. We don't want to penalize a strategy because the trades take longer to exit (we usually do place a minimum amount of executions for a strategy).

The way the sharp ratio works is the higher it is the better. So what i had done in the code is if there was not enough data to get a valid sharp ratio i just put the sharp ratio to a bad sharp ratio (in our repo it's set at 1 but in out VM i had changed it last minute to -20). The purpose was just to put it to a bad value, sharp ratios are good only if above 2.

Then i just did the negative of the sharp ratio we got (since we are minimizing and in reality we want to maximize), so if we actually had a strategy that gave us a 5 sharp ratio (really good) the minimization function would keep searching from that one..

Let me know if this makes sense, and really thanks a lot for the amazing work!

MTG-Mohsen commented 5 years ago

Another note for @xmatthias ,

I had added this line of code:

adding slippage of 0.1% per trade

    total_profit  = total_profit - 0.0005

Again not really clean the changes we made in the loss function but it was to explain to students and for them to change it if they want. But i think this is really important.

In stocks, or in the professional level of trading we back-test using quote data (not minute data). With quote data you can re-construct the order-book, and know exactly what price you could have bought at or sold at at any time (because you would have the bid/ask at that specific time). With minute data we don't have that, we just have the last executed trade at the nearest time to the candle end. This creates a problem because some coins in crypto have a 5% spread (difference between the bid and ask), so the code might think you can make 2% profit when in fact you would lose money if you got out aggressively on the other side of the spread.

So what we usually do is add some slippage that will account for the loss we would have by trying to get filled. Sometimes we have a slippage function that will have a variable amount based on volume..

I think the sharp ratio and this were the main things we played with, and the reason was just because i though they would be very beneficial, so adding them to the main freqtrade would be absolutely great!

xmatthias commented 5 years ago

I will definitely do what you suggested for future projects if we need a repository. Give us a few days to look into it and we're going to try to do it for this repository first +1

Don't feel forced, as mentioned i can understand the burden of maintaining a course (i have one myself, but not about trading), so if it takes weeks or months or never happens it's no problem for me.

@MTG-Mohsen Any consideration on not including risk-free rates in the calculation, which is usually seen in the calculations? (apart from not having it available within the loss-function)?

MTG-Mohsen commented 5 years ago

@xmatthias sorry for late reply, The risk free rate is only used in the sharp ratio when using it to compare long term mutual funds performance. Initially the sharp ratio was used for that, comparing mutual funds. (the best mutual funds in the world will have sharp ratios of about 1.5 max).

So when day trading we have to play with the formula a bit because we are looking at daily performance and not yearly. We eliminate the risk free rate (because it's negligible) and we multiply our result by the square root of trading periods (for example if the results we have are monthly we multiply by sqrt(12), if we have daily trading data for stocks we multiply by sqrt(252) since there is 252 trading days in the year for stocks, so for crypto i just multiplied by the sqrt(365)). This annualized the sharp ratio so we can compare strategies that are even across different asset classes/strategies etc. In day trading anything below a sharp ratio of 2 isn't worth looking at. We usually want >2.5 at least.

xmatthias commented 5 years ago

I've taken the liberty to rename this to "questions about sharpe ratio" - since that's more fitting now.

Thanks a lot for explaining - makes sense (to me) now.

sidenote: in the develop version, it's now possible to use --hyperopt-loss SharpeHyperOptLoss for hyperopt, which will load (and use) your calculation as loss function.

hroff-1902 commented 5 years ago

I didn't really understand the loss function that was there

Me too... 😆

hroff-1902 commented 5 years ago

@MTG-Mohsen Hi there,

I'm one of the current freqtrade devs, involved into hyperoptimization. We've borrowed your implementation of the Sharpe Ratio and included it in the main codebase (into the develop branch in the meantime). Don't you mind? (So that we have an explicite answer and be free of any licensing questions...)

Then, first of all, I'm not a professional trader, but a kind of mathematician (degree in Applied Maths in 90ths, trying to recall all that stuff I learned at that time for math. stat,, probablility and chaos theories, etc., trading cryptos in the meantime and enhancing Freqtrade... 😃 )

I have some doubts on your implementation of the Sharpe Ratio. The classic Sharpe ratio uses data aggregated for time periods -- for days, or, maybe, for shorter timeframes, on the hour (or even on timeframe) basis... Am I right here?

In your implementation you use the list of trades and it's not aggregated. The difference is the std. deviation, calculated on the set of data and used in the calculation of SR. I can illustrate: assume we have 2 days of trades with 4 trades, in day 1 we have trades with profit 0 and profit 14, in day 2 we have trades with profit 14 and 0. In our list of trades during backtesting you'll get it as (as the list of profits) [0, 14, 14, 0]. So the std. deviation calculated on this set of returns will also be 14.0. However, if you aggregate the trades on the daily basis, the list of daily returns will be [14, 14] and the std. deviation, accordingly, will be 0.0. And the SR calculated will be different responsibly. That's the point and the difference...

Ideally, I'd like to have an answer from the practitioning traders, if it really has sense to calculate SR on an unaggregated list of returns. I think it has, since the difference is only in the std. deviation, i.e. in the denominator, while numerator for the set is the same...

MTG-Mohsen commented 5 years ago

Don't you mind? (So that we have an explicite answer and be free of any licensing questions...)

Of course i don't mind! Anything we add here please use and add to the original code as you please!

MTG-Mohsen commented 5 years ago

Ideally, I'd like to have an answer from the practitioning traders, if it really has sense to calculate SR on an unaggregated list of returns. I think it has, since the difference is only in the std. deviation, i.e. in the denominator, while numerator for the set is the same...

You are absolutely right. The calculation should be done on the list of AGGREGATED trades. So in our case they would have to be on the daily returns. This means we've made a mistake, i probably thought those were daily returns (or maybe wasn't thinking while changing it). Sorry about that :/ You have it right!

ilyasst commented 5 years ago

Alright, I think it's time for that emergency update to let everybody know then :)

Thanks for double checking this @hroff-1902 👍

hroff-1902 commented 5 years ago

@MTG-Mohsen Thanks alot for answering! The current function, even if it produces results different from the classic SR, can be effectively used as a hyperopt objective, I guess.

Aggregating data can be a problem for shorter timeranges,