feat: add plot_params dict to plot_pytrendy for customisation#122
feat: add plot_params dict to plot_pytrendy for customisation#122gyr0tron wants to merge 2 commits into
Conversation
|
📚 Docs preview deployed! Your docs preview is available at: https://russellsb.github.io/pytrendy/pr-122/
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
RussellSB
left a comment
RussellSB
left a comment
There was a problem hiding this comment.
Thanks Simmar. Appreciate you going into way more than just figsize and title. I've left some nitpick comments re the API structure, docstring format for dict, and hard setting legend ncols.
| import matplotlib.patches as mpatches | ||
|
|
||
| def plot_pytrendy(df: pd.DataFrame, value_col: str, segments_enhanced: list[dict], suppress_show: bool = False) -> plt.Figure: | ||
| def plot_pytrendy(df: pd.DataFrame, value_col: str, segments_enhanced: list[dict], suppress_show: bool = False, plot_params: dict = None) -> plt.Figure: |
There was a problem hiding this comment.
Can you also define this in the main detect_trends() call? E.g. what was attempted in the stale PR. This way it would be more accessible to the user. The dict plot_params can be passed through the main access point detect_trends, to the internal plot_pytrendy function.
| plot_params (dict, optional): | ||
| Dictionary of plotting parameters to customise the figure. | ||
| Supported keys are 'figsize' (tuple), 'title' (str), 'xlabel' (str), 'ylabel' (str), 'colors' (dict of direction to color), 'alpha' (float), 'grid' (dict), 'legend_loc' (str), 'legend_bbox_to_anchor' (tuple), 'legend_ncol' (int). | ||
| Defaults to None (uses internal defaults). |
There was a problem hiding this comment.
can you re-format this docsting to e.g. how method_params was done? It just ends up looking nicer in the API Reference docs, e.g.: https://russellsb.github.io/pytrendy/pr-122/reference/pytrendy/detect_trends/
| 'grid': {'visible': True, 'which': 'major', 'color': 'gray', 'alpha': 0.3}, | ||
| 'legend_loc': 'upper right', | ||
| 'legend_bbox_to_anchor': (1, 1.15), | ||
| 'legend_ncol': 4, |
There was a problem hiding this comment.
I dont think legend_ncol should be customisable since Up, Down, Flat, Noise is currently always set to be length 4 for legend_handles . Im not sure what would happen if you set it to e.g. 6 or 2, but I assume it would break.
2 participants
plot_paramsdict support toplot_pytrendy.pyplot_pytrendyusage as default params are still in placeChanges
plot_pytrendysignature to acceptplot_paramstest_plot_pytrendy_core.pyto acceptplot_paramsand added a new test verifying that custom figsize and title work.Notes
Currently, only a limited set of colours are supported (only the ones supported by matplotlib starting with the keyword 'light'). This could be made into a new issue on its own to dynamically process lighter versions of colours that user specifies for fill and darker versions of respective colours for texts and shapes if the user only specifies a base colour e.g. red, blue, etc.
Issue
Closes #68