Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

unwanted change in spacing for marginal x and y plots #1862

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

Open
joelostblom wants to merge 4 commits into plotly:main
base: main
Choose a base branch
Loading
from joelostblom:marginal-spacing

Conversation

@joelostblom
Copy link
Contributor

@joelostblom joelostblom commented Nov 3, 2019

Previously, the marginal y plot was closer to the main plot compared to the marginal x plot:

image

After this change, both the marginal plots appear to be as far from the main plot:
image

I also increased the distance of both plots a tiny bit over the previous max distance because I think it looks better, but happy to change that if you disagree. The improvement is clearer (but still subtle) when using axes lines and a white background:

Old max:
image

New max:
image

Copy link
Contributor Author

The failing test seems unrelated to the changes I made.

Copy link
Contributor

While I’m not opposed to tweaking these values, please note that the actual physical spacing in the output depends on the height and width of the whole figure, as well as anything that pushes margins, such as tick labels, legends, color bars, multi-line titles etc... it’s going to be hard to claim that one pair of values makes the spacing "equal" in more than a few specific cases ;)

Copy link
Contributor Author

I see, I tested with a few different widths initially but didn't go extreme enough to notice a difference. Just tried it with more than twice the width compared to height and I can see what you mean.

I made a new commit that normalized based on the figure size. It looks good for me, so if you like it and don't any negative implications elsewhere, I can make the change to the other three places as well and drop the previous commits.

Copy link
Contributor

where does the 600 come from?

Copy link
Contributor Author

That's the value I used for width and height when I optimized the distance between the margins and main plot to an arbitrary value that I thought looked good (0.012). So now it will adjust this value based on the figure size.

It doesn't look perfect and could be adjusted to account for legends changing the effective width of the plot, but the principle works. Both the plots below look good with a square 600x600, but changes differently when elongated.

Without adjustment (much different)
image

With adjustment (still quite even)
image

Copy link
Contributor Author

Rebased

Copy link
Contributor Author

Not sure how to deal with autoscaling figures since their width is not known until the fig is created, which means that the marginal plot spacing can't be calculated before the fig is created.

@gvwilson gvwilson self-assigned this Jun 14, 2024
@gvwilson gvwilson removed their assignment Aug 2, 2024
@gvwilson gvwilson changed the title (削除) Even spacing for marginal x and y plots (削除ここまで) (追記) unwanted change in spacing for marginal x and y plots (追記ここまで) Aug 12, 2024
@gvwilson gvwilson added bug something broken P3 backlog regression this used to work labels Aug 12, 2024

row_heights = [main_size] * (nrows - 1) + [1 - main_size]
vertical_spacing = 0.01
vertical_spacing = 0.012*600/args["height"]
Copy link
Collaborator

@marthacryan marthacryan Aug 16, 2024

Choose a reason for hiding this comment

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

If we're checking for width being None below, shouldn't we check for height being None here?

Copy link
Collaborator

@marthacryan marthacryan Aug 16, 2024

Choose a reason for hiding this comment

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

Also, instead of multiplying two constants, could you just use 7.2?

Copy link
Contributor Author

@joelostblom joelostblom Aug 16, 2024

Choose a reason for hiding this comment

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

Thanks for your comments @marthacryan . I'm a bit removed from this PR since it was a while since I created it, but your suggestions here and in the other comment sounds good so feel to make those edits (any others you see fit) directly on this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

/me waves at @joelostblom

Copy link
Contributor Author

@joelostblom joelostblom Aug 19, 2024

Choose a reason for hiding this comment

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

Haha, hello Greg! Nice to see you here =)


column_widths = [main_size] * (ncols - 1) + [1 - main_size]
horizontal_spacing = 0.005
width_adjustment = 1 if args["width"] is None else 600 / args["width"]
Copy link
Collaborator

@marthacryan marthacryan Aug 16, 2024

Choose a reason for hiding this comment

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

For readability / ease of code comprehension, I'd love to have:

  • A comment for both of these explaining why these constants are being used here and why we're adjusting for width/height
  • More consistency between the width and the height adjustments. It wasn't immediately clear to me that they were doing the same thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marthacryan marthacryan marthacryan left review comments

+1 more reviewer

@gvwilson gvwilson gvwilson left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Labels

bug something broken P3 backlog regression this used to work

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /