8000 Update MovieWriter dpi default by heathhenley · Pull Request #8063 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Update MovieWriter dpi default #8063

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 16, 2017
Merged

Update MovieWriter dpi default #8063

merged 2 commits into from
Feb 16, 2017

Conversation

heathhenley
Copy link
Contributor

Adds default dpi=None argument to *MovieWriter classes, and uses figure.dpi as the default if dpi is None.

This is related to issue #7616. @tacaswell please advise if this approach is what you had in mind for resolving the issue.

I'm a new contributor and I wasn't exactly sure the best way to unit test a change like this. However, I created a test that calls setup with dpi as the default, and makes sure that dpi is set to the figure.dpi. I did not add a change log entry because it is such a small change, please let me know if I should.

tacaswell
tacaswell reviewed Feb 11, 2017
View reviewed changes
lib/matplotlib/animation.py Outdated
@@ -165,7 +165,7 @@ def setup(self, fig, outfile, dpi):
The filename of the resulting movie file
dpi: int
The DPI (or resolution) for the file. This controls the size
in pixels of the resulting movie file.
in pixels of the resulting movie file. Default is figure.dpi.
Copy link
Member

Choose a reason for hiding this comment

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

The docstring on this needs , optional added

Copy link
Member

Choose a reason for hiding this comment

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

probably on all of the docstrings actually.

@heathhenley
Copy link
Contributor Author

@tacaswell I just updated the docstrings.

I just noticed that in Animate.save() has dpi=None default to rcParam['savefig.dpi'], and only if this is 'figure' it is set to figure.dpi. Should I use that behavior instead of setting None to figure.dpi?

@tacaswell
Copy link
Member

On one hand what you have here is simpler and does not depend on global state, on the other hand reaching out to rcParams['savefig.dpi'] is more consistent with savefig and Animation.save.

I am leaning towards not defaulting to rcParams['savefig.dpi']. This is a lower level interface (most users should be using Animation.save which is why this was a required argument before) and is being used by sophisticated users are are rolling their own version of save. Coupling into our global state starts to be an annoying detail to work around rather than a helpful default. It would not take much to convince me I am wrong on this and it should look at 'savefig.dpi'.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 11, 2017
@tacaswell tacaswell changed the title Update MovieWriter dpi default [MRG+1] Update MovieWriter dpi default Feb 11, 2017
dopplershift
dopplershift reviewed Feb 15, 2017
View reviewed changes
Copy link
Contributor
@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

I'm 👍 on the change--just the question about the docs.

lib/matplotlib/animation.py Outdated
The DPI (or resolution) for the file. This controls the size
in pixels of the resulting movie file.
in pixels of the resulting movie file. Default is figure.dpi.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too?

lib/matplotlib/animation.py Outdated
The DPI (or resolution) for the file. This controls the size
in pixels of the resulting movie file.
in pixels of the resulting movie file. Default is figure.dpi.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should figure.dpi be fig.dpi since fig is the name of the argument?

lib/matplotlib/animation.py Outdated
The dpi of the output file. This, with the figure size,
controls the size in pixels of the resulting movie file.
Default is figure.dpi.
Copy link
Contributor

Choose a reason for hiding this comment

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

And here?

@heathhenley
Copy link
Contributor Author

@tacaswell @dopplershift Thanks, I updated in (c2aac89). Let me know if should rebase this on master/squash some of these commits to something cleaner.

@dopplershift
Copy link
Contributor

If it's not a lot of effort, I'd prefer to see this squashed down a bit.

@heathhenley heathhenley force-pushed the Update_MovieWriter_dpi_default branch from c2aac89 to 47ef9df Compare February 16, 2017 17:09
@heathhenley heathhenley force-pushed the Update_MovieWriter_dpi_default branch from 47ef9df to e0c002c Compare February 16, 2017 17:32
@heathhenley
Copy link
Contributor Author

@dopplershift I squashed all the lints/nits into the original commit so it looks like it never happened. I also rebased on the current master because I was a few commits behind.

dopplershift
dopplershift approved these changes Feb 16, 2017
View reviewed changes
Copy link
Contributor
@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Pending one last run of CI.

@NelleV NelleV changed the title [MRG+1] Update MovieWriter dpi default [MRG+2] Update MovieWriter dpi default Feb 16, 2017
@dopplershift dopplershift merged commit f697e8c into matplotlib:master Feb 16, 2017
@dopplershift dopplershift changed the title [MRG+2] Update MovieWriter dpi default Update MovieWriter dpi default Feb 16, 2017
@dopplershift
Copy link
Contributor

Thanks @heath730 !

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

@tacaswell tacaswell tacaswell approved these changes

@dopplershift dopplershift dopplershift approved these changes

Assignees
No one assigned
Labels
topic: animation
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@heathhenley @tacaswell @dopplershift
0