Lumiwealth / quantstats_lumi

Apache License 2.0
53 stars 20 forks source link

log_scale on graphs in 0.3.1 causes y-axis label issues #43

Closed kartiksubbarao closed 6 days ago

kartiksubbarao commented 1 month ago

In 0.3.1 some graphs, including the Worst 5 Drawdown Periods graph, have been changed to log_scale by default. This has the unfortunate side effect of removing several y-axis labels. Here is an example. In 0.3.0, there are multiple y-axis labels ranging from 15% to -15% in 5% intervals, whereas in 0.3.1, there are only 3 labels -- 17%, 0% and -15%:

version 0.3.0: image

version 0.3.1: image

grzesir commented 1 month ago

Any idea how to fix this?

kartiksubbarao commented 1 month ago

I did some searching and found a stackoverflow post that describes one method to display labels on a log-scale axis: https://stackoverflow.com/a/30890025

For the Drawdowns graph in particular though, I would recommend going back to the 0.3.0 behavior of not using log-scale. The lowest drawdown is, after all, -100% -- just two orders of magnitude. Given that there are other graphs in the tearsheet that show the returns in log-scale, I think it would be better to keep the drawdowns graph as linear scale for clear visualization (or perhaps make log_scale an optional argument). But it's your call of course.

hygarlic commented 1 month ago

I don't think we have to roll back the code. The code zoom in the curves as it should be. The solution, imho, involves playing with the ticks on the y-axis.

Have a look at the ticker api: https://matplotlib.org/stable/api/ticker_api.html

For instance, in _plotting/core.py, after: ax.set_ylim(bottom=min_val, top=max_val)

add: import matplotlib.ticker as ticker ax.yaxis.set_major_locator(ticker.MaxNLocator(nbins=10))

I'm not saying this the right solution, as now there are 10 ticks but with a random interval. But it probably involves playing with the ticker api. Maybe ticker.MaxNLocator is not the correct function to use, another function in the ticker api could fix the problem.

hygarlic commented 1 month ago

We should aim to keep the returns in log-scale, as it is standard in backtesting. The log-scale really helps to have a good understanding in the "Worst 5 drawdowns".

grzesir commented 1 month ago

@kartiksubbarao Do these solutions work for you?

kartiksubbarao commented 1 month ago

@grzesir It's hard to say without seeing where/how the new labels are placed. In 0.3.0 with linear scale, the labels are automatically placed at even intervals by matplotlib (5% in this case), which looks good to me. Whereas for a log chart, someone would have to recompute those intervals explicitly in plot_longest_drawdowns().

If you do decide to make log scale the default for the drawdowns charts, I would recommend explicitly mentioning it in the graph title for clarity, just as Log Scale is labeled in the other graphs.

More broadly speaking, I suspect I'm not the only user who prefers linear scale for the drawdowns charts. And there are likely other users like @hygarlic who prefer log-scale. It might be useful to expose the log_scale parameter to the drawdowns_periods() function back at the top-level html() function, perhaps with an interface like this:

html(returns, benchmark, drawdowns_periods={'log_scale':False})

That would involve some more work though. Just some thoughts.

hygarlic commented 1 month ago

Yes, "(Log Scaled)" is missing in the title of "Worst 5 Drawdowns Periods".

Using an optional parameter to change the default plotting to a log scale is interesting. We just need to think it through thoroughly for all plots. For instance, "Cumulative Returns VS Benchmark" and "Cumulative Returns VS Benchmark (Log Scaled)" provide the same information in two different scales. I'm not interested in having the first one. So, I would be interested in a parameter to turn off the linear-scaled graph.

The simpler solution would be to have either Log Scale or Linear Scale for all returns charts. By default, it would be Log Scale. When we start to mix up Log or Linear Scale for some plots, it starts to be more complex. Having all returns charts in one mode (Log or Linear) is consistent by design. This is what I would suggest.

What do you think? If you're ok with this, I can make the patch.

grzesir commented 1 month ago

If you want all graphs to be one or the other then I think they should all be linear. After teaching this stuff to many hundreds of people I’ve seen how confusing log charts can be to some people.

The alternative is to have both log and linear.

Either works for me. If you submit a patch I can approve/merge it.

Robert Grzesik 347-635-3416

On Sat, Jun 22, 2024 at 4:28 AM hygarlic @.***> wrote:

Yes, "(Log Scaled)" is missing in the title of "Worst 5 Drawdowns Periods".

Using an optional parameter to change the default plotting to a log scale is interesting. We just need to think it through thoroughly for all plots. For instance, "Cumulative Returns VS Benchmark" and "Cumulative Returns VS Benchmark (Log Scaled)" provide the same information in two different scales. I'm not interested in having the first one. So, I would be interested in a parameter to turn off the linear-scaled graph.

The simpler solution would be to have either Log Scale or Linear Scale for all returns charts. By default, it would be Log Scale. When we start to mix up Log or Linear Scale for some plots, it starts to be more complex. Having all returns charts in one mode (Log or Linear) is consistent by design. This is what I would suggest.

What do you think? If you're ok with this, I can make the patch.

— Reply to this email directly, view it on GitHub https://github.com/Lumiwealth/quantstats_lumi/issues/43#issuecomment-2183915691, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIYQK7QQR55BBWL7HSYMELZIUYTZAVCNFSM6AAAAABJQYVGWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBTHEYTKNRZGE . You are receiving this because you were mentioned.Message ID: @.***>

hygarlic commented 1 month ago

Plotting both linear and logarithmic charts for all returns nearly doubles the number of charts, making it too cumbersome. I can make a patch for the returns chart to be either all linear or all logarithmic, based on a global parameter. Linear will be the default setting, as you've requested.

hygarlic commented 1 month ago

See PR: https://github.com/Lumiwealth/quantstats_lumi/pull/46

hygarlic commented 1 week ago

This has been fixed in 0.3.2.

Ticket can be closed.

grzesir commented 5 days ago

So how can we bring back the main log chart? Is there an option for it now? Looking for only the one log chart vs the benchmark

Robert Grzesik 347-635-3416

On Wed, Jul 24, 2024 at 10:22 AM Kartik Subbarao @.***> wrote:

Closed #43 https://github.com/Lumiwealth/quantstats_lumi/issues/43 as completed.

— Reply to this email directly, view it on GitHub https://github.com/Lumiwealth/quantstats_lumi/issues/43#event-13631446313, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIYQK2I5FPVC4IGBVISD6TZN62CVAVCNFSM6AAAAABJQYVGWSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGYZTCNBUGYZTCMY . You are receiving this because you were mentioned.Message ID: @.***>

hygarlic commented 4 days ago

According to the specs in https://github.com/Lumiwealth/quantstats_lumi/pull/46 :

"1. new optional global boolean setting log_scale to plot all returns either in Log or Linear Scale. Default log_scale=False (linear scale)"

All returns plots are either in Log or in Linear Scale. Currently no option to mix between those scales.

Amateurs have Linear Scale by default and Pros can use Log scales. This way the type of plots is clear.