Conversation
The failing test seems unrelated to the changes I made. |
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 ;) |
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. |
where does the 600 come from? |
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 ( 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) With adjustment (still quite even) |
e0bc12f to 2e3ed30 Compare Rebased |
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. |
| @@ -1330,7 +1330,7 @@ def init_figure(args, subplot_type, frame_list, nrows, ncols, col_labels, row_la | |||
| main_size = 0.84 | |||
| row_heights = [main_size] * (nrows - 1) + [1 - main_size] | |||
| vertical_spacing = 0.01 | |||
| vertical_spacing = 0.012 * 600 / args["height"] | |||
There was a problem hiding this comment.
If we're checking for width being None below, shouldn't we check for height being None here?
There was a problem hiding this comment.
Also, instead of multiplying two constants, could you just use 7.2?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Haha, hello Greg! Nice to see you here =)
| @@ -1343,7 +1343,8 @@ def init_figure(args, subplot_type, frame_list, nrows, ncols, col_labels, row_la | |||
| main_size = 0.84 | |||
| 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"] | |||
There was a problem hiding this comment.
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
Previously, the marginal y plot was closer to the main plot compared to the marginal x plot:
After this change, both the marginal plots appear to be as far from the main plot:
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:
New max:
