Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure multiline and grouped bar chart functions #20

Merged
merged 24 commits into from
Apr 22, 2024
Merged

Conversation

MelinaGoula
Copy link
Contributor

Functions to review:

  • horizontal_grouped_bar_chart
  • vertical_grouped_bar_chart
  • multi_linechart_test
  • helper functions (lines 1102 to 1490)

@MelinaGoula
Copy link
Contributor Author

Comparison of stacked bar chart functions

Previous (stacked_chart)
sphx_glr_plot_grouped_bar_rick_001
Rewrite (horizontal_grouped_bar_chart)
sphx_glr_plot_grouped_bar_rick_002
Example of (vertical_grouped_bar_chart)
sphx_glr_plot_grouped_bar_rick_003

@MelinaGoula
Copy link
Contributor Author

Functionality that was not carried over from multi_linechart to multi_linechart_test (the temporary new version):

  • Ability to set custom styling parameters (font-size, font-family, fontfamily-list, stroke, stroke-width, border)
  • Setting a plot title
  • Dashed grid lines as opposed to solid grid lines
  • Customizable tick length and width
  • Customizable sizes for axis and tick labels
  • Customizable precision for tick labels
  • Ability to set a specific line colour, width and border style
  • Option to add shaded areas

Example plot with multi_linechart showing a lot of the functionality
sphx_glr_plot_multiline_003

Recreated with multi_linechart_test:
sphx_glr_plot_multiline_002
I am not sure how to deal with legend placement in cases like this where there is not a lot of free space. One alternative is to follow the previous format of having multiple columns in the legend which would look like this:
sphx_glr_plot_multiline_002

rick.py Show resolved Hide resolved
rick.py Outdated

delta = (ymax - ymin)/4
i = 0
while True:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be using calculate_delta? Same comment on line 580.

rick.py Outdated Show resolved Hide resolved
@gabrielwol
Copy link
Contributor

I am not sure how to deal with legend placement in cases like this where there is not a lot of free space. One alternative is to follow the previous format of having multiple columns in the legend which would look like this:

Have you considered having the legend always outside the plot? This could help with consistency no matter the data being plotted!

@gabrielwol
Copy link
Contributor

Functionality that was not carried over from multi_linechart to multi_linechart_test (the temporary new version):
....
Recreated with multi_linechart_test:
302367434-a85466f0-fda7-46b1-983d-2c67ca25f31e

I think the ticks would be an important feature for this graph type which we often use for timeseries. In our demos people are always trying to pinpoint the specific date that graphed events happen. Maybe dark ticks for the labelled points and lighter ticks for the others as a default?

@gabrielwol
Copy link
Contributor

@MelinaGoula your 3 new charting functions are very elegant compared with the old versions, well done! See a few comments above.

@MelinaGoula
Copy link
Contributor Author

MelinaGoula commented Feb 27, 2024

I have made the following changes and I think this is ready for re-review:

  • added the option to add minor ticks to the multi line chart function and specify their spacing, e.g.
    image
  • moved the legend to the outside of plots (for both grouped bar and multiline plots)
    image
  • added the option for labeled shaded areas, e.g. (for multiline plots)
    image

One thing I think can be done in the future to improve the code structure is to not have as many function parameters passed in the helper functions, but this can be probably only be done by refactoring the code.

@chmnata chmnata removed their request for review February 27, 2024 21:32
Copy link
Contributor

@gabrielwol gabrielwol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look great! Well done @MelinaGoula

Copy link
Member

@radumas radumas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got a slight misfire in building Sphinx.

We need the docs/.nojekyll file to remain.

Also looks like we don't need to commit .doctree files (can add this to .gitignore)

The contents of the html folder should be in docs.

Maybe we should change the build config back to building to sphinx/build and have instructions for people to manually copy the contents of html over to docs when they are ready to merge

Copy link
Member

@radumas radumas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of changes here but I think this is really going to help clean up the repo!

@radumas radumas merged commit a7bdfa7 into main Apr 22, 2024
@MelinaGoula MelinaGoula deleted the update-rick.py branch August 28, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants