support dark mode #88
Conversation
WalkthroughAdds dark-mode support across plotting and report generation: a _DarkModeContext for Matplotlib, a new dark_mode parameter threaded through reports, wrappers, and core plotting functions, dark-mode CSS in the report template, and a version bump to 1.0.2. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Reports as reports.py
participant Wrappers as plotting wrappers
participant Core as _plotting.core
participant MPL as Matplotlib
participant HTML as report.html/CSS
User->>Reports: generate report(dark_mode=True)
Reports->>Wrappers: call wrapper plots with dark_mode=True
Wrappers->>Core: call core plot functions with dark_mode=True
Core->>Core: enter _DarkModeContext(dark_mode=True)
Core->>MPL: apply dark rcParams / styles
MPL-->>Core: render figures
Core->>MPL: restore rcParams (exit context)
Core-->>Wrappers: return figures/snippets
Wrappers-->>Reports: embed figures
Reports->>HTML: add "dark-mode" class to <body>
Reports-->>User: return HTML (dark-mode styling active)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
quantstats_lumi/report.html (2)
298-304: Hardcoded color in.metric-titlenot using CSS variable.The
.metric-titleelements inside.metrics-headerand.metric-boxuse hardcodedcolor: #999instead ofvar(--light-text), which breaks consistency in dark mode.🔎 Apply this diff to use CSS variables:
.metrics-header .metric-title { font-size: 16px; - color: #999; + color: var(--light-text); text-align: center; font-weight: 400; margin-bottom: 10px; } .metric-box .metric-title { font-size: 16px; - color: #999; + color: var(--light-text); margin-bottom: 5px; font-weight: 400; }Also applies to: 325-330
394-400: Hardcoded color in.info-iconnot using CSS variable.The
.info-iconuses hardcodedcolor: #999which won't adapt to dark mode.🔎 Apply this diff:
.info-icon { font-size: 0.8em; - color: #999; + color: var(--light-text); margin-left: 5px; vertical-align: middle; cursor: pointer; }quantstats_lumi/_plotting/wrappers.py (2)
1081-1108:monthly_returnswrapper doesn't supportdark_modeparameter.The
monthly_returnsfunction wrapsmonthly_heatmapbut doesn't expose or pass thedark_modeparameter, making it inconsistent with other wrappers.🔎 Apply this diff to add dark_mode support:
def monthly_returns( returns, annot_size=10, figsize=(10, 5), cbar=True, square=False, compounded=True, eoy=False, grayscale=False, + dark_mode=False, fontname="Arial", ylabel=True, savefig=None, show=True, ): return monthly_heatmap( returns=returns, annot_size=annot_size, figsize=figsize, cbar=cbar, square=square, compounded=compounded, eoy=eoy, grayscale=grayscale, + dark_mode=dark_mode, fontname=fontname, ylabel=ylabel, savefig=savefig, show=show, )
941-1051: Add colorbar text styling for dark mode.The dark mode implementation handles the heatmap background, axes text colors, and tick styling well. However, the colorbar's tick labels remain unstyled and will be unreadable against the dark background. Style the colorbar after the heatmap creation by setting tick label colors to white when
dark_mode=True.
🧹 Nitpick comments (8)
quantstats_lumi/reports.py (2)
132-135: Consider edge case if body tag already has attributes.The string replacement assumes
<body>appears exactly as-is. If the template's body tag ever has existing attributes (e.g.,<body class="existing">), this replacement won't match. This is likely fine given you control the template, but worth noting for future template changes.
660-671: Consider addingdark_modesupport tofull(),basic(), andplots()functions.The
dark_modeparameter is only added to thehtml()function. Thefull(),basic(), andplots()functions call plotting functions directly without passingdark_mode. If dark mode support is desired for non-HTML outputs (e.g., Jupyter notebooks with dark themes), consider propagating the parameter to these functions as well.quantstats_lumi/report.html (1)
29-43: Well-structured dark mode CSS implementation.The dark mode styling is properly implemented using CSS variable overrides. The color choices provide good contrast for readability.
One minor consideration: the
.no-trades-messageselector (lines 500-509) still uses hardcoded colors (#fff3cd,#856404,#ffeeba) that won't adapt to dark mode. Consider adding dark mode variants for this warning style.🔎 Suggested addition for dark mode warning styling:
.no-trades-message { background-color: #fff3cd; color: #856404; border: 1px solid #ffeeba; border-radius: 6px; padding: 12px 15px; margin-bottom: 20px; text-align: center; font-weight: 500; } + + body.dark-mode .no-trades-message { + background-color: #3d3000; + color: #ffc107; + border-color: #665200; + }quantstats_lumi/_plotting/core.py (4)
737-738: Context manager wraps only figure creation, not styling operations.Unlike
plot_timeserieswhich wraps most operations inside_DarkModeContext, here only_plt.subplots()is wrapped while subsequent styling (lines 799-832) happens outside the context. While this works because rcParams affect the figure at creation time, it's inconsistent with other functions in this file.Consider wrapping the entire plotting section for consistency, or document this pattern difference.
186-257: Consider extracting repeated dark mode styling into a helper function.The dark mode styling pattern (setting tick colors, spine colors, legend styling) is repeated across multiple functions (
plot_returns_bars,plot_timeseries,plot_histogram,plot_rolling_stats,plot_rolling_beta,plot_longest_drawdowns,plot_distribution).Consider extracting this into a helper function to reduce duplication:
🔎 Example helper function:
def _apply_dark_mode_axis_styling(ax, legend=None): """Apply dark mode styling to axis and optional legend.""" ax.tick_params(colors="white") ax.xaxis.label.set_color("white") ax.yaxis.label.set_color("white") for spine in ax.spines.values(): spine.set_color("white") if legend is not None: for text in legend.get_texts(): text.set_color("white") legend.get_frame().set_facecolor('#1a1a1a') legend.get_frame().set_edgecolor('white') legend.get_frame().set_alpha(0.8)
646-656: Unused variablexfrom histplot return value.The return value from
_sns.histplotis assigned toxbut never used (also flagged at line 629). Consider removing the assignment if the return value isn't needed.🔎 Apply this diff:
- x = _sns.histplot( + _sns.histplot( data=combined_returns, x="Returns", bins=bins,
589-589: Lambda assigned to variable - consider usingdefinstead.Per static analysis hint E731, assigning a lambda to a variable is discouraged. Use a named function for clarity.
🔎 Apply this diff:
- fix_instance = lambda x: x[x.columns[0]] if isinstance(x, _pd.DataFrame) else x + def fix_instance(x): + return x[x.columns[0]] if isinstance(x, _pd.DataFrame) else xquantstats_lumi/_plotting/wrappers.py (1)
55-256:snapshotfunction doesn't support dark mode.The
snapshotfunction creates a multi-panel summary plot but doesn't havedark_modesupport. While this might be intentional scope limitation for this PR, it creates an inconsistency in the API where some visualization functions support dark mode and others don't.Consider adding this in a follow-up or documenting which functions support dark mode.
Would you like me to open a follow-up issue to track adding dark mode support to
snapshot,earnings, andmonthly_returns_detailedview?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)CHANGELOG.rst(1 hunks)meta.yaml(1 hunks)quantstats_lumi/_plotting/core.py(19 hunks)quantstats_lumi/_plotting/wrappers.py(29 hunks)quantstats_lumi/report.html(5 hunks)quantstats_lumi/reports.py(19 hunks)quantstats_lumi/version.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
quantstats_lumi/_plotting/core.py
273-274: try-except-pass detected, consider logging the exception
(S110)
273-273: Do not catch blind exception: Exception
(BLE001)
278-279: try-except-pass detected, consider logging the exception
(S110)
278-278: Do not catch blind exception: Exception
(BLE001)
332-332: Avoid specifying long messages outside the exception class
(TRY003)
474-475: try-except-pass detected, consider logging the exception
(S110)
474-474: Do not catch blind exception: Exception
(BLE001)
479-480: try-except-pass detected, consider logging the exception
(S110)
479-479: Do not catch blind exception: Exception
(BLE001)
589-589: Do not assign a lambda expression, use a def
Rewrite fix_instance as a def
(E731)
646-646: Local variable x is assigned to but never used
Remove assignment to unused variable x
(F841)
671-671: Unused lambda argument: loc
(ARG005)
688-689: try-except-pass detected, consider logging the exception
(S110)
688-688: Do not catch blind exception: Exception
(BLE001)
693-694: try-except-pass detected, consider logging the exception
(S110)
693-693: Do not catch blind exception: Exception
(BLE001)
1139-1140: try-except-pass detected, consider logging the exception
(S110)
1139-1139: Do not catch blind exception: Exception
(BLE001)
1144-1145: try-except-pass detected, consider logging the exception
(S110)
1144-1144: Do not catch blind exception: Exception
(BLE001)
1253-1253: Unused lambda argument: loc
(ARG005)
1266-1267: try-except-pass detected, consider logging the exception
(S110)
1266-1266: Do not catch blind exception: Exception
(BLE001)
1270-1271: try-except-pass detected, consider logging the exception
(S110)
1270-1270: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
.gitignore (1)
20-21: LGTM!Adding
quantstats_lumi.egg-infoto.gitignoreis appropriate for excluding Python packaging artifacts.quantstats_lumi/version.py (1)
1-1: LGTM!Version bump to 1.0.2 aligns with the dark mode feature addition documented in the changelog.
meta.yaml (2)
2-2: LGTM!Version updated to 1.0.2, consistent with
version.pyandCHANGELOG.rst.
10-10: Verify the sha256 hash after publishing to PyPI.The
sha256hash should be updated to match the new 1.0.2 tarball once it's published to PyPI. The current hash may be from a previous version.quantstats_lumi/reports.py (2)
67-67: LGTM!The
dark_modeparameter is properly typed with a sensible default ofFalsefor backwards compatibility.
374-374: LGTM!The
dark_modeparameter is consistently propagated to all plotting function calls throughout thehtml()function.quantstats_lumi/_plotting/core.py (1)
53-93: Context manager implementation looks good, with one concern about thread safety.The
_DarkModeContextcorrectly saves and restores matplotlib rcParams. However,_plt.rcParamsis a global mutable object, so concurrent usage in multi-threaded environments could cause race conditions.For this use case (report generation), this is likely acceptable, but worth documenting or considering if concurrency becomes a requirement.
quantstats_lumi/_plotting/wrappers.py (1)
370-416: LGTM - dark_mode parameter properly propagated.The
returnswrapper correctly adds thedark_modeparameter and passes it through to_core.plot_timeseries. This pattern is consistently applied across all updated wrapper functions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
quantstats_lumi/_plotting/core.py (1)
750-755: Context manager scope issue inplot_rolling_stats.The figure is created inside the
with _DarkModeContext(dark_mode):block, but subsequent operations (setting spines, plotting, etc.) occur outside it. This means the dark mode rcParams are reverted before most of the figure configuration happens.🔎 Apply this diff to fix the context manager scope:
text_color = "white" if dark_mode else "black" subtitle_color = "#cccccc" if dark_mode else "gray" bg_color = "#1a1a1a" if dark_mode else "white" - with _DarkModeContext(dark_mode): - fig, ax = _plt.subplots(figsize=figsize) + fig, ax = _plt.subplots(figsize=figsize) ax.spines["top"].set_visible(False)The context manager is not needed here since all styling is done explicitly. Similar issue exists in
plot_rolling_beta(lines 903-904).
🧹 Nitpick comments (1)
quantstats_lumi/_plotting/core.py (1)
333-335: Potential issue:fillnawithinplace=Truemodifies input data.Using
inplace=Trueon the inputreturnsandbenchmarkSeries modifies the caller's data, which may cause unexpected side effects.🔎 Apply this diff to avoid mutating input:
- returns.fillna(0, inplace=True) + returns = returns.fillna(0) if isinstance(benchmark, _pd.Series): - benchmark.fillna(0, inplace=True) + benchmark = benchmark.fillna(0)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
quantstats_lumi/_plotting/core.py(25 hunks)quantstats_lumi/_plotting/wrappers.py(48 hunks)quantstats_lumi/report.html(9 hunks)quantstats_lumi/reports.py(44 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
quantstats_lumi/_plotting/wrappers.py (1)
quantstats_lumi/_plotting/core.py (1)
monthly_heatmap_detailedview(1406-1564)
quantstats_lumi/_plotting/core.py (2)
quantstats_lumi/_plotting/wrappers.py (1)
returns(400-452)quantstats_lumi/stats.py (1)
compsum(41-43)
🪛 Ruff (0.14.8)
quantstats_lumi/_plotting/core.py
276-277: try-except-pass detected, consider logging the exception
(S110)
276-276: Do not catch blind exception: Exception
(BLE001)
281-282: try-except-pass detected, consider logging the exception
(S110)
281-281: Do not catch blind exception: Exception
(BLE001)
338-338: Avoid specifying long messages outside the exception class
(TRY003)
480-481: try-except-pass detected, consider logging the exception
(S110)
480-480: Do not catch blind exception: Exception
(BLE001)
485-486: try-except-pass detected, consider logging the exception
(S110)
485-485: Do not catch blind exception: Exception
(BLE001)
681-681: Unused lambda argument: loc
(ARG005)
698-699: try-except-pass detected, consider logging the exception
(S110)
698-698: Do not catch blind exception: Exception
(BLE001)
703-704: try-except-pass detected, consider logging the exception
(S110)
703-703: Do not catch blind exception: Exception
(BLE001)
1160-1161: try-except-pass detected, consider logging the exception
(S110)
1160-1160: Do not catch blind exception: Exception
(BLE001)
1165-1166: try-except-pass detected, consider logging the exception
(S110)
1165-1165: Do not catch blind exception: Exception
(BLE001)
1278-1278: Unused lambda argument: loc
(ARG005)
1291-1292: try-except-pass detected, consider logging the exception
(S110)
1291-1291: Do not catch blind exception: Exception
(BLE001)
1295-1296: try-except-pass detected, consider logging the exception
(S110)
1295-1295: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (14)
quantstats_lumi/report.html (2)
29-43: LGTM! Clean CSS variable-based dark mode implementation.The dark mode styling uses CSS custom properties effectively, allowing for easy theming. The color choices provide good contrast for readability (e.g.,
#e0e0e0text on#1a1a1abackground).
511-515: Good addition of dark mode override for the no-trades message.The override ensures the warning message remains visible and appropriately styled in dark mode with adjusted background (
#3d3000) and text (#ffc107) colors.quantstats_lumi/_plotting/core.py (3)
53-93: Well-structured context manager for dark mode theming.The
_DarkModeContextproperly saves and restores matplotlib rcParams, preventing theme leakage between plots. The implementation correctly handles the case whendark_mode=Falseby doing nothing.One minor observation: the context manager could benefit from a
try/finallypattern in__exit__to ensure restoration even if an exception occurs, though current usage appears safe.
153-155: Consistent enforcement of mutual exclusivity.Good pattern for ensuring
grayscaleanddark_modedon't conflict. This is applied consistently across all plotting functions.
253-260: Good dark mode legend styling.The legend frame styling for dark mode (facecolor, edgecolor, alpha) ensures readability against the dark background. This pattern is consistently applied across plotting functions.
quantstats_lumi/reports.py (4)
67-95: Well-documented dark_mode parameter addition.The docstring properly describes the new parameter with type, default value, and purpose. The parameter is correctly placed in the function signature.
685-697: Dark mode parameter properly threaded tofull()function.The
dark_modeparameter is correctly added and passed to theplots()call.
1450-1454: Dark mode parameter added toplots()function.The parameter is correctly added to the function signature and will be used for matplotlib-based visualizations.
135-160: Regex-based dark mode class injection is functional with correct pattern matching.The code correctly implements dark mode CSS class injection. The regex pattern
class="([^"]*)"properly matches class attributes including empty ones likeclass=""(the[^"]*quantifier matches zero or more non-quote characters).The single-quote handling that follows double-quote replacement is a defensive measure for edge cases. In well-formed HTML, attributes use consistent quoting, so both quote styles won't coexist in a single tag. The current template uses
<body>without attributes, so the code works correctly for the default case.Since custom templates are supported via the
template_pathparameter, consider documenting any constraints or recommendations for custom templates to ensure the dark mode injection works as expected across different template variations.quantstats_lumi/_plotting/wrappers.py (5)
55-73: Consistent dark mode implementation insnapshot().The mutual exclusivity check and color variable setup follow the established pattern. This is correctly replicated across all wrapper functions.
236-245: Good per-axis dark mode styling in loops.Applying dark mode styling to each axis in the loop ensures consistent appearance across all subplots.
1009-1026: Dark mode styling applied correctly to monthly heatmap.The background colors, text colors, and tick parameters are properly set for dark mode. The heatmap colorbar styling is handled appropriately.
1161-1168: Mutual exclusivity and delegation to core function.The
monthly_returns_detailedviewwrapper correctly enforces grayscale/dark_mode mutual exclusivity and delegates to the core implementation with thedark_modeparameter.
400-450: Dark mode parameter correctly passed to core plotting function.The
returns()wrapper properly passesdark_modeto_core.plot_timeseries(), ensuring the theming is applied in the underlying implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
quantstats_lumi/_plotting/core.py (2)
153-155: Consider using a local variable instead of modifying the parameter.The pattern
if grayscale: dark_mode = Falseappears throughout (lines 153-155, 327-329, 522-524, 742-744, etc.). While functionally correct, modifying parameter values is not ideal style.🔎 Optional improvement
Consider using a local variable:
- # Grayscale and dark_mode are mutually exclusive - if grayscale: - dark_mode = False + # Grayscale and dark_mode are mutually exclusive + use_dark_mode = dark_mode and not grayscaleThen use
use_dark_modethroughout the function instead ofdark_mode.
194-298: Consider standardizing the dark mode application pattern.The codebase uses two different approaches for dark mode:
- Context manager approach:
plot_timeseries,plot_histogram,plot_returns_bars,plot_longest_drawdowns,plot_distributionuse_DarkModeContext- Manual styling approach:
plot_rolling_stats,plot_rolling_beta,monthly_heatmap_detailedviewmanually set colors without the context managerAdditionally, when the context manager is used, figure creation is sometimes inside the context (cleaner, as in
plot_timeseriesline 362) and sometimes outside (requiring manual facecolor setting, as inplot_returns_barsline 183).Standardizing to always use
_DarkModeContextand create figures inside the context would improve maintainability and reduce code duplication. For example:with _DarkModeContext(dark_mode): fig, ax = _plt.subplots(figsize=figsize) # All styling code here automatically benefits from dark mode rcParamsAlso applies to: 330-502, 746-833, 1086-1180, 1227-1310
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
quantstats_lumi/_plotting/core.py(26 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
quantstats_lumi/_plotting/core.py (2)
quantstats_lumi/_plotting/wrappers.py (1)
returns(400-452)quantstats_lumi/stats.py (1)
compsum(41-43)
🪛 Ruff (0.14.8)
quantstats_lumi/_plotting/core.py
276-277: try-except-pass detected, consider logging the exception
(S110)
276-276: Do not catch blind exception: Exception
(BLE001)
281-282: try-except-pass detected, consider logging the exception
(S110)
281-281: Do not catch blind exception: Exception
(BLE001)
338-338: Avoid specifying long messages outside the exception class
(TRY003)
480-481: try-except-pass detected, consider logging the exception
(S110)
480-480: Do not catch blind exception: Exception
(BLE001)
485-486: try-except-pass detected, consider logging the exception
(S110)
485-485: Do not catch blind exception: Exception
(BLE001)
681-681: Unused lambda argument: loc
(ARG005)
698-699: try-except-pass detected, consider logging the exception
(S110)
698-698: Do not catch blind exception: Exception
(BLE001)
703-704: try-except-pass detected, consider logging the exception
(S110)
703-703: Do not catch blind exception: Exception
(BLE001)
1158-1159: try-except-pass detected, consider logging the exception
(S110)
1158-1158: Do not catch blind exception: Exception
(BLE001)
1163-1164: try-except-pass detected, consider logging the exception
(S110)
1163-1163: Do not catch blind exception: Exception
(BLE001)
1276-1276: Unused lambda argument: loc
(ARG005)
1289-1290: try-except-pass detected, consider logging the exception
(S110)
1289-1289: Do not catch blind exception: Exception
(BLE001)
1293-1294: try-except-pass detected, consider logging the exception
(S110)
1293-1293: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
quantstats_lumi/_plotting/core.py (2)
53-94: LGTM! Clean context manager implementation.The
_DarkModeContextclass properly implements the context manager protocol with appropriate save/restore logic for matplotlib rcParams. Color choices (#1a1a1a background, white text, #666666 grid) provide good contrast for dark mode.
190-193: LGTM! Thoughtful color choices for dark mode visibility.The color adjustments demonstrate good attention to contrast and readability:
- Using orange instead of red for average lines (line 676)
- Lighter red (#ff6666) with increased alpha for drawdown highlights (lines 1111-1112)
- Consistent legend styling with appropriate frame colors (lines 254-260)
Also applies to: 254-260, 368-370, 635-635, 676-676, 1111-1112
|
should be good from here.. the nitpicks mention are pretty unneeded |
1 participant
Adds
dark_modeflag to reports. Defaults to false and should be backwards compatible. Custom themeing is applied to maintain proper contrast and visibility.Such request solves something such as ranaroussi/quantstats#492
Mainly done for my personal desires.
Comparison Example:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.