refactor: Subplot layout adjustments #181
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@baggiponte tagging you here as the first fixes are ready for review, looks like I am not able to add a reviewer yet. :) |
|
Sorry for the late reply. Still under pressure, will try to review this ASAP. |
|
Starting review, might still take a few days. First of all: excellent job, high quality comments, names and abstractions over repetitive stuff. Will leave some comments here and there. I would kindly ask you to add some tests for the functions you added. Feel free to place them under a new |
|
Kudos for removing We do that "iterate over entities" at least three times in the module. Would you mind making that a standalone function and add tests? Of course, available to help. You don't need to test the tests, but make sure that the |
There was a problem hiding this comment.
I am also giving a quick review, as @baggiponte called me in (I feel like a reviewer-ad-honorem 😂).
I left a minor suggestion - as such function has no facing api, but still good for standardization.
As @baggiponte mentioned, I aswell think there are other tiny code parts that can be refactored into a utility function.
Regarding unit tests, sadly I never had to test plotting functionalities.
I wouldn't test the plots, but some properties, e.g. number of rows and cols in this case. |
|
Sounds great, thanks both for taking the time. I will look into these over the weekend! |
|
Thanks! One more thing: I updated main, could you rebase your PR? We'd prefer that to merging master, to keep a simpler commit history. |
|
@baggiponte @FBruzzesi, I have now tried to module some of the repetitive code to own functions, but not sure what is the best way to deal with the small differences in plotting (naming, colors etc.). I've now added plot_params dict prior calling the "_add_scatter_traces_to_subplots".. maybe I just made it more complicated than it was haha :D I've tried with multiple different len series and columns and plotting looks to work well now with plot_panel, plot_forecast, and plot_backtests. I also added the tests to the test_plotting.py, have a look! |
|
To add, I also tested some of the other plotting functions and noticed that the plots using plotly express doesn't work no more and leads to AssertionError. Not quite sure what causes it but most likely something changed either in plotly or polars. In order to make it work now, I rewrote the plot_entities to go.Figure instead, and for other plot functions made a quick fix and converting the polars DF to pandas prior plotting with .to_pandas() |
Adjusted the subplot position calculation to correctly handle an arbitrary number of series and columns. This change ensures that the grid layout of subplots is dynamically adjusted to accommodate the specified number of series, thus avoiding IndexError and incorrect subplot arrangements. Fixes functime-org#179
Detailed Description: - Introduced new function _set_subplot_default_kwargs to dynamically set default subplot sizes, ensuring fixed sizing and improved readability. - Made corrections in plot_panel function to fix the same issue previously addressed in plot_forecasts, enhancing consistency across plotting functions. - Adjusted overall subplot layout to include additional space for titles and other elements, improving plot aesthetics.
- Added a check in plot_panel, plot_forecast, and plot_backtests functions to ensure n_series does not exceed the number of unique entities in the dataset. - This change prevents potential errors and ensures more reliable behavior when the input n_series parameter is higher than the available unique entities. - Minor refactoring for improved readability and consistency across these functions by adding _calculate_subplot_n_rows function to calculate the rows based on n_series and n_columns
Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
Refactor common data preparation steps in plot_panel, plot_forecasts, and plot_backtests into a shared preprocessing function. This change improves code maintainability and reduces duplication by centralizing the logic.
- Extracted similar scatter subplot plotting logic into a centralized function
- Created tests_plotting.py module - Testing various utility functions and different cases
- Created tests based on wrong index loop which is now corrected back to original - Minor format and comment changes
- Instead of int | None using Union[int, None] to avoid error in python 3.8
- Plotly express plots raising AssertError. Something has changed either in Polars or Plotly but that doesn't seem to work no more. - Changed plot entities to go.Figure instead - For other px. elements converting data_frame to pandas prior working makes it work.
miroslaavi
force-pushed
the
enhancement/subplot-layout-adjustments
branch
from
255031d
to
8e0635e
Compare
Cool, never done that prior as mostly used version history for my own projects that I was working alone. Also, noticed that my commit messages are not very constant or following a clear structure, so I'll check some best practices for that too. :) Now it is like 10 commits for these changes, is it preferred to keep that way or is it ideal to squeeze in one commit for the merge? |
|
Ciao Miro, sorry for the late reply. Have a workshop tomorrow at EPFL. Should have time to review afterwards. |
|
All good and take your time, have a great workshop! :) |
baggiponte
changed the title
|
@baggiponte, thanks! This was quite many changes in one PR, but going forward will raise issues and focus on smaller chunks. This is good to go from my side now. |
|
Amazing, will merge soon. Indeed it's a big PR but we requested most of the changes. Thank you very much for your contribution and patience 🫡 |
Fixes #179
What does this implement/fix? Explain your changes.
This pull request introduces a new function _set_subplot_default_kwargs to dynamically set default plot sizes for subplots, enhancing the readability and aesthetics of plots in the plotting module. This function is now utilized in plot_panel, plot_forecasts, and plot_backtests to ensure consistent and optimal sizing of plots, especially when the user hasn't specified height and width in kwargs.
Additionally, this PR includes a fix for the #179 for the subplot functions plot_forecast, plot_panel, and plot_backtests.
Any other comments?
Also, added a logic if n_series is greater than the max unique entities, then use max unique entities instead for plotting to avoid an error.