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

Improve handling of large cascade plots #71

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

Pennycook
Copy link
Contributor

Related issues

Closes #39.

Proposed changes

  • Switch to AA, AB, AC, AD labels for very large plots, increasing the upper limit to 676 platforms.
  • Tweak default plotting parameters for large plots to make them readable.

Clearly 676 platforms is too many, and I don't think anybody will ever want to produce such a large plot. But this change allows us to handle plots with many platforms without crashing. There will still be cases where the plot is unreadable, but the user can work around that by increasing the plot size.

Here's an example of 30 platforms, where the default is now readable:

30

By the time we hit 50 platforms, things are pretty squished:

50

I'm pretty happy with this as a fix for now, because most studies I've seen in real life use somewhere between 1-10 platforms. Fixing this for the general case will require some assistance from somebody who has a better grasp of how things like font sizes and plot sizes are connected in matplotlib.

Use a single function to generate platform labels for both matplotlib
and pgfplots backends.

Signed-off-by: John Pennycook <[email protected]>
Two letter labels are spreadsheet-style: AA, AB, AC, AD...

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook added the enhancement New feature or request label Sep 16, 2024
@Pennycook Pennycook added this to the 1.0.0 milestone Sep 16, 2024
Copy link
Contributor

@laserkelvin laserkelvin left a comment

Choose a reason for hiding this comment

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

Just left a note on the incomplete docstring, LGTM otherwise



def _get_platform_labels(platforms: list[str]) -> dict[str, str]:
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing argument in docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6471f60.

Add missing Parameters section.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook merged commit ff6226a into intel:main Sep 25, 2024
7 checks passed
@Pennycook Pennycook deleted the 26-platform-limit branch September 25, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cascade plot crashes with more than 26 platforms
2 participants