fixed conversion from pt to inch in tight_layout #1065

Merged
merged 2 commits into from Aug 15, 2012

Projects

None yet

2 participants

@pwuertz
Contributor
pwuertz commented Aug 10, 2012

When using tight_layout the figure padding changes for different figure.dpi settings. This is caused by an error in when converting from points to inches. A point is always 1/72 inch and does not depend on the dots per inches.

Member

Ooooh, good catch! I suspect this may also require updating test images. We probably should also double-check this in axes_grid1 in the mpl_toolkits.

Member

looks like axes_grid1 is unaffected, I think.

Member

Indeed, the tests for tight_layout fail with this PR (which isn't a bad thing!). Note that in my test runs (using an older freetype), the layout is "less tight" now than before. This may mean that we need to adjust some of the default parameters.

Contributor
pwuertz commented Aug 10, 2012

Hm, at least for my matplotlib installation the default figure.dpi is 80. I assume this is the default for the tests as well? If so, the borders will grow by 80/72 = 11% after applying this patch. You think it's best to reduce the default tight_layout defaults by that fraction?

Do you get an additional difference for other freetype libraries or is your experience explained by that 11% fraction?

Contributor
pwuertz commented Aug 10, 2012

Ok, when I change all ocurences of pad=1.2 to pad=1.08 all tight_layout tests pass except tight_layout6_svg, but this seems to be a simple rounding error. I added another commit that includes these new defaults and updates the expected image. Shall I combine these commits into one for the pull request?

Member

No need to combine commits into one (actually, I prefer multiple commits in a PR as it helps me to see what actions were taken to address various comments). I will test this branch out a bit and double-check for any lingering references to the old padding values.

Contributor
pwuertz commented Aug 14, 2012

Rebased the commits on current master and fixed another pad=1.2 default in tight_layout.py that has been added by a recent commit. Also, correcting the padding value in test_tightlayout.py instead of uploading a new test image made the patch less noisy ;).

Member

The tests pass for me (except for knownfails due to my freetype version). Merging...

@WeatherGod WeatherGod merged commit e6818e7 into matplotlib:master Aug 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment