Lumiwealth / quantstats_lumi

Apache License 2.0
53 stars 20 forks source link

Add detailed monthly returns heatmap with drawdown information #45

Closed twkim112 closed 1 week ago

twkim112 commented 1 month ago

KakaoTalk_Photo_2024-06-26-16-56-50

Key changes:

This new visualization provides a comprehensive view of strategy performance, allowing users to quickly identify patterns, strong/weak periods, and the relationship between returns and drawdowns over time.

korbit-ai[bot] commented 1 month ago

My review is in progress :book: - I will have feedback for you in a few minutes!

hygarlic commented 1 month ago

There seem to have an issue with the monthly maxDD: in your example heatmap, october 2023 return is -7.67% but the maxDD for the month is only -6.37%.

hygarlic commented 1 month ago

Another thing, in the left margin, return is for the whole year, but MDD is only the worst monthy return. It should be instead the maxDD for the whole year.

grzesir commented 1 month ago

Max drawdown should not be any time period at all, it should be the biggest drop the strategy had irrespective of how long it took for the drop to happen. Isn’t that how it is now?

Robert Grzesik 347-635-3416

On Wed, Jun 26, 2024 at 12:26 PM hygarlic @.***> wrote:

Another thing, in the left margin, return is for the whole year, but MDD is only the worst monthy return. It should be instead the maxDD for the whole year.

— Reply to this email directly, view it on GitHub https://github.com/Lumiwealth/quantstats_lumi/pull/45#issuecomment-2192115336, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIYQKYD6ZGIH2SYKE5WL6LZJLTTLAVCNFSM6AAAAABJ5LIJ2SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSGEYTKMZTGY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

twkim112 commented 1 month ago

Thank you for your feedback. I'll adjust the MDD to show the maximum drawdown for the entire year, matching the annual return figure. Regarding the monthly maxDD calculation I am going to fix code especially for October 2023 to correct the discrepancy you noted.

grzesir commented 1 month ago

I don’t think the max drawdown should be adjusted at all. The max drawdown is not over a month, a year or anything else. It is intended to be the maximum loss, no matter the time period. For example, the SPY had a max drawdown recently of around 30% which took almost two years.

As for the monthly max dd, which figures do you mean exactly? And what is the issue here?

Robert Grzesik 347-635-3416

On Wed, Jun 26, 2024 at 7:31 PM Taewan Kim @.***> wrote:

Thank you for your feedback. I'll adjust the MDD to show the maximum drawdown for the entire year, matching the annual return figure. Regarding the monthly maxDD calculation I am going to fix code especially for October 2023 to correct the discrepancy you noted.

— Reply to this email directly, view it on GitHub https://github.com/Lumiwealth/quantstats_lumi/pull/45#issuecomment-2192786524, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIYQKYAWPO6SGC2NG55YC3ZJNFMDAVCNFSM6AAAAABJ5LIJ2SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSG44DMNJSGQ . You are receiving this because you commented.Message ID: @.***>

hygarlic commented 1 month ago

on the x-axis:

on the y-axis:

hygarlic commented 1 month ago

I would suggest to introduce a global setting in htm() to show either Monthy Returns (current table), or the new Monthy Returns & Drawdown (this PR). With the current Monthy Returns without Drawdowns as the default case.

hygarlic commented 1 month ago

The new percentage scale on the right could be reused for the Monthly Returns (current table without Drawdowns).

hygarlic commented 1 month ago

For the title, instead of: "Strategy - Monthly Returns (%) & Drawdowns" it should be: "Strategy - Monthly Returns & Drawdowns (%)"

hygarlic commented 1 month ago

On the left margin, "Return:" takes some horizontal space and is repeated multiple times, same for "MDD:". The same names should not be repeated.

Both Return and MDD on the left are for Year-To-Date (YTD).

Suggestion:

  1. drop on the left margin for all years "Return:" and "MDD:". From the title it is clear that there are two information "Returns" and "Drawdowns".
  2. Add "()" for the value of the YTD MDD on the left margin for consistency with the monthly MDD values in the table.
  3. Add at the bottom left this name for the whole column: "YTD". This makes clear that Returns and MDD are given YTD.
twkim112 commented 1 month ago

I've addressed most of the comments in the feedback. When I checked the returns file and looked at the period from the end of September to October 2023, I found that the return calculated from the last trading day of September to the last trading day of October is -7.67%, which is why it's displayed that way. However, the monthly drawdown was shown as -6.37% because it was based on October 1st to 31st. I've modified the function to calculate the monthly drawdown on a month-end to month-end basis, consistent with the returns calculation. I've also implemented the other suggested changes from the feedback.

image

hygarlic commented 1 month ago

Maybe 'YTD' right-aligned instead of left-aligned, same as the years on the left.

hygarlic commented 1 month ago

I still think that using a setting in the main function htm() to show either 'Monthly Returns' (current table) or the new 'Monthly Returns & Drawdown' (this PR) is advisable, as some users may not need to see monthly drawdowns. Default would be without drawdowns.

Adding drawdowns make the table less readable for users who are primarily interested in monthly returns.

hygarlic commented 1 month ago

Maybe a font size slightly smaller for all drawdowns compared to returns, inside the table and on the left, for increased readability.

twkim112 commented 1 month ago

I agree. I don't think this feature is particularly necessary for a basic HTML report. This code was created to view monthly drawdowns separately when generating specific plots.

twkim112 commented 1 month ago

image

I've been making several attempts and have made some progress, but I'm still encountering a couple of issues:

I've moved the YTD label to the right-aligned position, which matches the reference image.

hygarlic commented 1 month ago

"When I reduce the font size of the monthly drawdown, the font color doesn't match the return value color as intended.": this is weird.

Can you add a little space in the table between Returns and monthly DD as they are too close? Like 1 or 2 px. Just inside the table.

twkim112 commented 4 weeks ago
for i in range(pivot_returns.shape[0]):
    for j in range(pivot_returns.shape[1]):
        cell = ax.get_children()[i * pivot_returns.shape[1] + j + 1] 
        return_color = cell.get_color()
        monthly_dd_color = 'white' if return_color == 'w' else 'black'
        ax.text(j + 0.5, i + 0.55, annot_drawdowns.iloc[i, j],
                ha='center', va='top', fontsize=annot_size * 0.8, color=monthly_dd_color)

When I added this code, it produced a plot that looks correct as shown below. I'm still unsure how to apply different font sizes to yticklabels in seaborn, particularly making the drawdown percentage smaller than the year and return percentage in the YTD section.

image

twkim112 commented 4 weeks ago

image

  1. Y-axis label and drawdown display have been adjusted for better alignment and clarity.
  2. The YTD (Year-to-Date) label position has been modified to ensure it's properly aligned with other elements.
  3. Both monthly and yearly drawdown font sizes are now set to 80% of the return font size.
  4. The base annotation size annot_size has been increased to 11, providing a better default size for the heatmap's text elements.
hygarlic commented 4 weeks ago

It looks nice.

Do you think it is possible to print monthly drawdowns in the table in grey instead of either black or white? Same as in the left margin? This would help separate monthly returns and drawdowns.

twkim112 commented 4 weeks ago

image

For the existing white font tile, I set it to the gainsboro color because if I changed this colors dimgray, it would look bad on certain tiles. The following plot shows an example with all monthly dd set to dimgray.

image

hygarlic commented 4 weeks ago

Yes, first one with gainsboro instead of dimgray everywhere is better.

Now that the distinction between Returns and Drawdowns is made very clear (smaller font and greyish), the question is: should the parentheses be dropped?

Could you try removing the parentheses for all Drawdowns, in the table and on the left margin?

twkim112 commented 4 weeks ago

image

It seems like () is needed for negative numbers like in financial statements, but I don't think it's necessary since there is an explicit - sign. The version without the () looks a bit cleaner, though.

twkim112 commented 4 weeks ago
hygarlic commented 4 weeks ago

Thanks, without () drawdowns look much cleaner.

After checking with the original Monthly Returns (%) chart, returns were provided inside the table without %. Why adding % everywhere in the table and in the left margin?

The (%) information is already given twice: in the title and in the scale on the right. It is better to stick with the original design and drop % everywhere inside the table and on the left margin.

twkim112 commented 4 weeks ago

image

The cumulative returns of the strategy I used show many empty spaces without % because the period starts from 2019, making it look sparse. However, as you mentioned, if the period gets longer, it look strange with too many %.

hygarlic commented 4 weeks ago

BTC_monthy_returns

This is the BTC monthly returns from the original layout. As you can see, the Returns fully fill the tiles. Your layout of the returns inside the table should fully fill the tiles and match the original layout.

hygarlic commented 4 weeks ago

Same for the YEARS and MONTHS, they should be of the same size

hygarlic commented 4 weeks ago

Same for the title.

hygarlic commented 4 weeks ago

There are some thin vertical grey bars for the remainder of 2024 starting from July. Can you remove them?

twkim112 commented 3 weeks ago

image

Regarding the issue of overlapping returns and drawdowns in the YTD section for long-term data like BTC, I experimented with reducing the font size when overlaps occur. However, this approach led to two problems: the font became too small compared to the returns in the central heatmap, and overall readability was significantly compromised. Instead of reducing font size of it, I think it's better that users adjust the figsize argument to accommodate longer time periods. This approach maintains readability while allowing for more data to be displayed clearly. At this point, I believe this is the most suitable solution, as it gives users the flexibility to optimize the display for their specific data range.

figsize = (14, 6), return_font_rate = 1.0 image

figsize = (16, 9), return_font_rate = 1.0 image

twkim112 commented 3 weeks ago

I realized that sns.heatmap doesn't automatically adjust the font size, but simply uses rcParams['font.size']. So I rolled back to the previous format. I kept the feature where users can either use annot_size as is or adjust it with return_font_rate.

twkim112 commented 3 weeks ago
twkim112 commented 3 weeks ago
hygarlic commented 3 weeks ago

Maybe the years in the left margin could be slightly bigger than the returns.

hygarlic commented 3 weeks ago

The way missing months are handled with empty tiles instead of showing titles with zeroes is very nice.

Could you please also open another PR just to patch the original layout with the same: not showing missing months instead of filling them with zeroes?

However, if a monthly return is really zero but not missing, it should be kept.

twkim112 commented 3 weeks ago

Thanks for the suggestions. I propose merging this PR for now. I'll create a new pull request for these improvements for future implementation when I have more bandwidth, including your suggestions and an update of the original heatmap.

BlackArbsCEO commented 2 weeks ago

this is a great addition. any ideas when this can get merged?

grzesir commented 2 weeks ago

I will take a look at this soon and merge this + any other PRs. Been injured the past week so I’m running behind

Robert Grzesik 347-635-3416

On Wed, Jul 10, 2024 at 11:06 AM Brian @.***> wrote:

this is a great addition. any ideas when this can get merged?

— Reply to this email directly, view it on GitHub https://github.com/Lumiwealth/quantstats_lumi/pull/45#issuecomment-2220760620, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIYQK5S7NXB3L766OJAD4LZLVEXBAVCNFSM6AAAAABJ5LIJ2SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRQG43DANRSGA . You are receiving this because you commented.Message ID: @.***>

grzesir commented 1 week ago

this is awesome @twkim112 ! great work! merging now

grzesir commented 1 week ago

This is now deployed in v0.3.2! 🎉

hygarlic commented 1 week ago

@twkim112

Since your PR has been merged, could you please patch the original heatmap with only your fix that handles missing months by simply not showing them instead of filling them with zeroes?

This would make it look much better. Looking forward to it!

Thanks!

twkim112 commented 1 week ago

I'll create a PR in the next few days!

hygarlic commented 1 week ago

@twkim112

And maybe in another PR, you could introduce in the original heatmap your color scale with percentages on the right side. Please be careful not to reduce the width of the heatmap table to maintain readability.

Thanks!

twkim112 commented 1 week ago

Okay I'll try adding the cbar option to the original heatmap and see if that affects the width

hygarlic commented 1 week ago

There is nothing in the right column next to the heatmap, so you should be able to add the color scale there in the margin and keep the heatmap table exactly as it is.