refactor: Subplot layout adjustments

[miroslaavi]

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.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
functime-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 23, 2024 5:10pm

[miroslaavi]

@baggiponte tagging you here as the first fixes are ready for review, looks like I am not able to add a reviewer yet. :)

[baggiponte]

Sorry for the late reply. Still under pressure, will try to review this ASAP.

[baggiponte]

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 tests/test_plotting.py. If you need help, do not hesitate to ask! @FBruzzesi is also much better than me at testing and I think he can provide some help.

[baggiponte]

Kudos for removing np.repeat usage - that thing bugged me for waaay to much, but I never had the time to clean that up.

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 divmod vs np.repeat behaves the same.

[baggiponte]

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.

[miroslaavi]

Sounds great, thanks both for taking the time. I will look into these over the weekend!

[baggiponte]

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.

[miroslaavi]

@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!

[miroslaavi]

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()

[miroslaavi]

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.

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?

[baggiponte]

Ciao Miro, sorry for the late reply. Have a workshop tomorrow at EPFL. Should have time to review afterwards.

[miroslaavi]

All good and take your time, have a great workshop! :)

[miroslaavi]

@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.

[baggiponte]

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 🫡