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

Adjust plotly x_label_height_factor #582

Merged
merged 7 commits into from
Oct 6, 2020

Conversation

lboeman
Copy link
Member

@lboeman lboeman commented Oct 1, 2020

  • Closes #xxxx .
  • I am familiar with the contributing guidelines.
  • Tests added.
  • Updates entries to docs/source/api.rst for API changes.
  • Adds descriptions to appropriate "what's new" file in docs/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Adjusted the X_LABEL_HEIGHT_FACTOR to work with the longest possible forecast name: A forecast name containing 13 4 letter words separated by spaces, + the 'Prob(f <= x) = 100.0%' labelling added by the plotting code. I hit a few cases where Plotly tried to angle the x axis labels, which caused some labels to run off the right side of the plot. So I've forced the plot to display x axis labels vertically when there are more than 6 labels. .

@lboeman lboeman requested a review from wholmgren October 1, 2020 22:02
@wholmgren
Copy link
Member

So I've forced the plot to display x axis labels vertically when there are more than 6 labels. .

Maybe I missed something in the code, but have you confirmed there's no problem with long names if there's only 4 or 5 labels?

@lboeman
Copy link
Member Author

lboeman commented Oct 1, 2020

So I've forced the plot to display x axis labels vertically when there are more than 6 labels. .

Maybe I missed something in the code, but have you confirmed there's no problem with long names if there's only 4 or 5 labels?

I took another look and reverted this behavior. I had not tested with the maximum length name, and it did still run off.

@lboeman
Copy link
Member Author

lboeman commented Oct 2, 2020

Here are a couple of screenshots of the extremes here. Most Probabilistic forecasts will probably break the 15 character check to trigger this adjustment due to the arbiter-added suffixes.
Short names:
shortname

Including longest possible forecast name:
longname

@wholmgren anything else here?

@wholmgren
Copy link
Member

wholmgren commented Oct 2, 2020

Seems like most forecasts will break the limit too, right? These are 24-25 including space and .

newplot(42)

I prefer the 45 degree labels to 90 degree labels, but most of all I'd rather have it work reliably.

Looks like we could add <br> tags to long names but maybe that's another can of worms.

@lboeman
Copy link
Member Author

lboeman commented Oct 2, 2020

Here's forcing 45 degree angles among a few different reports. Interestingly, setting the angle explicitly to 45 does not suffer from the same runoff issue as allowing Plotly to try it for itself.
Screenshot from 2020-10-02 13-07-03
Screenshot from 2020-10-02 13-14-17
Screenshot from 2020-10-02 13-14-35

@wholmgren
Copy link
Member

Interestingly, setting the angle explicitly to 45 does not suffer from the same runoff issue as allowing Plotly to try it for itself.

what!?!? known plotly bug?

@lboeman
Copy link
Member Author

lboeman commented Oct 5, 2020

Updated to explicitly set a tickangle of 90 when longest label > 60 characters and a tickangle of 45 for longest label between 30 and 60, and default to plotly behavior for shorter labels. This helps to minimize the squeezing of the plot up into the top left of the plot area.
The core of the issue can be found here: plotly/plotly.js#2704 (comment). In short, we need to supply enough explicit values to avoid the infinite adjustment loop that occurs when Plotly reshapes the axis/plot against each other.

I think this is the best mix of reliable and decent looking for most cases we're going to get until there's an upstream solution.

EXAMPLES:
Longest length/ 90 degree labels
Screenshot from 2020-10-05 10-41-28

Length > 30 characters/ 45 degree labels
Screenshot from 2020-10-05 10-40-55

Default behavior
Lots of names all shorter than 30 characters:
Screenshot from 2020-10-05 11-20-14

Names short enough that they can be printed horizontally, plot expands to fill the space.
Screenshot from 2020-10-05 11-22-17

I did also try adding line breaks to long names, but this causes more overlap with lots of forecasts with or without angled labels.

docs/source/whatsnew/1.0.0rc4.rst Outdated Show resolved Hide resolved
@wholmgren wholmgren added this to the 1.0 rc4 milestone Oct 6, 2020
@wholmgren wholmgren added the bug Something isn't working label Oct 6, 2020
@lboeman lboeman merged commit 1150173 into SolarArbiter:master Oct 6, 2020
@lboeman lboeman deleted the adjust-xlabel-offsets branch August 16, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants