Update MovieWriter dpi default by heathhenley · Pull Request #8063 · matplotlib/matplotlib

heathhenley · 2017-02-11T20:07:55Z

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 · 2017-02-11T21:03:11Z

lib/matplotlib/animation.py

        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.


The docstring on this needs , optional added

probably on all of the docstrings actually.

heathhenley · 2017-02-11T21:30:59Z

@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 · 2017-02-11T22:57:11Z

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'.

dopplershift

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

dopplershift · 2017-02-15T17:01:08Z

lib/matplotlib/animation.py

+        dpi : int, optional
            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.


dopplershift · 2017-02-15T17:01:28Z

lib/matplotlib/animation.py

+        dpi: int, optional
            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.


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

dopplershift · 2017-02-15T17:01:48Z

lib/matplotlib/animation.py

+        dpi : number, optional
            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.


heathhenley · 2017-02-16T01:12:42Z

@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 · 2017-02-16T15:51:55Z

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

heathhenley · 2017-02-16T17:35:56Z

@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

Pending one last run of CI.

dopplershift · 2017-02-16T23:41:07Z

Thanks @heath730 !

tacaswell reviewed Feb 11, 2017

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Feb 11, 2017

tacaswell approved these changes Feb 11, 2017

View reviewed changes

tacaswell changed the title ~~Update MovieWriter dpi default~~ [MRG+1] Update MovieWriter dpi default Feb 11, 2017

dopplershift reviewed Feb 15, 2017

View reviewed changes

dopplershift added the topic: animation label Feb 15, 2017

heathhenley force-pushed the Update_MovieWriter_dpi_default branch from c2aac89 to 47ef9df Compare February 16, 2017 17:09

heathhenley added 2 commits February 16, 2017 12:30

Set default dpi to figure.dpi for MovieWriter classes.

3026bbd

Add test for setup dpi default.

e0c002c

heathhenley force-pushed the Update_MovieWriter_dpi_default branch from 47ef9df to e0c002c Compare February 16, 2017 17:32

dopplershift approved these changes Feb 16, 2017

View reviewed changes

NelleV changed the title ~~[MRG+1] Update MovieWriter dpi default~~ [MRG+2] Update MovieWriter dpi default Feb 16, 2017

dopplershift merged commit f697e8c into matplotlib:master Feb 16, 2017

dopplershift changed the title ~~[MRG+2] Update MovieWriter dpi default~~ Update MovieWriter dpi default Feb 16, 2017

Uh oh!

Conversation

heathhenley commented Feb 11, 2017

Uh oh!

tacaswell Feb 11, 2017

Choose a reason for hiding this comment

Uh oh!

tacaswell Feb 11, 2017

Choose a reason for hiding this comment

Uh oh!

heathhenley commented Feb 11, 2017

Uh oh!

tacaswell commented Feb 11, 2017

Uh oh!

dopplershift left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

dopplershift Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

dopplershift Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

heathhenley commented Feb 16, 2017

Uh oh!

dopplershift commented Feb 16, 2017

Uh oh!

heathhenley commented Feb 16, 2017

Uh oh!

dopplershift left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift commented Feb 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants