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

added bbox_inches flag. Defaults to None #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joglekara
Copy link

@joglekara joglekara commented Apr 11, 2022

Related to #94

This may help users with eliminating whitespace and other alignment related issues.

Copy link
Contributor

@zmoon zmoon left a comment

Choose a reason for hiding this comment

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

bbox_inches and pad_inches can be modified via rcParams, so another way to resolve this could be to document how to do that.

@@ -274,6 +276,9 @@ def __init__(
Movie size.
dpi : int
Movie resolution.
bbox_inches: str
Same as bbox_inches flag in savefig from matplotlib.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Same as bbox_inches flag in savefig from matplotlib.
Passed to :meth:`~matplotlib.figure.Figure.savefig` when saving frames.

@@ -274,6 +276,9 @@ def __init__(
Movie size.
dpi : int
Movie resolution.
bbox_inches: str
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bbox_inches: str
bbox_inches: str or matplotlib.transforms.Bbox

@@ -274,6 +276,9 @@ def __init__(
Movie size.
dpi : int
Movie resolution.
bbox_inches: str
Same as bbox_inches flag in savefig from matplotlib.
Bounding box in inches: only the given portion of the figure is saved. If 'tight', try to figure out the tight bbox of the figure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Bounding box in inches: only the given portion of the figure is saved. If 'tight', try to figure out the tight bbox of the figure.
Bounding box in inches: only the given portion of the figure is saved.
If ``'tight'``, try to figure out the tight bbox of the figure.
Try this option to eliminate unnecessary amounts of edge whitespace in frames.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should pad_inches also be added?

Copy link
Owner

@jbusecke jbusecke left a comment

Choose a reason for hiding this comment

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

Oh so sorry I didnt even realize that this PR was already submitted.
In general I am ok with it, but I wonder if we can generalize the input, so that the user can change any kwarg on .savefig() without us having to copy everyone of them?

@@ -253,6 +254,7 @@ def __init__(
pixelwidth=1920,
pixelheight=1080,
dpi=200,
bbox_inches=None,
Copy link
Owner

Choose a reason for hiding this comment

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

We might add other kwargs for .savefig() here (see comments below). If so should we have some dict that catches all save_kwargs=None (we can then set defaults we like in the init)? Otherwise we might be accumulating a bunch of input arguments and the signature of this might become unruly?

@@ -285,6 +290,7 @@ def __init__(
self.pixelwidth = pixelwidth
self.pixelheight = pixelheight
self.dpi = dpi
self.bbox_inches = bbox_inches
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. I would prefer to carry self.save_kwargs here as a dict.

@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

Merging #95 (8d656ea) into master (8c565d8) will increase coverage by 1.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   77.53%   78.59%   +1.05%     
==========================================
  Files           3        3              
  Lines         325      327       +2     
  Branches       61       61              
==========================================
+ Hits          252      257       +5     
+ Misses         48       46       -2     
+ Partials       25       24       -1     
Flag Coverage Δ
unittests 78.28% <100.00%> (+0.74%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xmovie/core.py 85.20% <100.00%> (+1.69%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants