Skip to content

Animation Writer changed to PillowWriter in animate_line and animate_imshow#52

Merged
johnomotani merged 16 commits into
boutproject:1d-animationsfrom
rdoyle45:1d-animations
Oct 15, 2019
Merged

Animation Writer changed to PillowWriter in animate_line and animate_imshow#52
johnomotani merged 16 commits into
boutproject:1d-animationsfrom
rdoyle45:1d-animations

Conversation

@rdoyle45

@rdoyle45 rdoyle45 commented Sep 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Writer change requested by @TomNicholas in pull request #36

@pep8speaks

pep8speaks commented Sep 11, 2019 •
edited
Loading

Copy link
Copy Markdown

Hello @rdoyle45! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-14 10:56:13 UTC

@codecov-io

codecov-io commented Sep 11, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #52 into 1d-animations will increase coverage by 18.05%.
The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           1d-animations      #52       +/-   ##
==================================================
+ Coverage          43.72%   61.78%   +18.05%     
==================================================
  Files                  6        6               
  Lines                279      280        +1     
  Branches              59       59               
==================================================
+ Hits                 122      173       +51     
+ Misses               147       78       -69     
- Partials              10       29       +19
Impacted Files Coverage Δ
xbout/plotting/animate.py 63.63% <100%> (+54.4%) ⬆️
xbout/boutdataarray.py 76.66% <0%> (+50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 317420e...bfad80e. Read the comment docs.

rdoyle45
rdoyle45 commented Sep 11, 2019
View reviewed changes
Comment thread xbout/plotting/animate.py Outdated
@rdoyle45 rdoyle45 closed this Sep 11, 2019
@rdoyle45 rdoyle45 reopened this Sep 11, 2019
@rdoyle45 rdoyle45 changed the title Animation Writer changed to PillowWriter in animate1D and animate2D Animation Writer changed to PillowWriter in animate_line and animate_imshow Sep 11, 2019
rdoyle45
rdoyle45 commented Sep 12, 2019
View reviewed changes
Comment thread xbout/plotting/animate.py Outdated
@TomNicholas

TomNicholas commented Sep 12, 2019

Copy link
Copy Markdown
Collaborator

Thanks Rhys!

Only comment - presumably we should also update the required version of matplotlib to >= 3.1 to match?

@rdoyle45
Update to required Matplotlib version
ef8f211 
To allow for the use of PillowWriter in plotting/animate.py
@rdoyle45

rdoyle45 commented Sep 12, 2019

Copy link
Copy Markdown
Collaborator Author

Thanks Rhys!

Only comment - presumably we should also update the required version of matplotlib to >= 3.1 to match?

Yes, good call. Changes made.

@rdoyle45 rdoyle45 changed the title Animation Writer changed to PillowWriter in animate_line and animate_imshow [WIP] Animation Writer changed to PillowWriter in animate_line and animate_imshow Sep 12, 2019
@rdoyle45 rdoyle45 changed the title [WIP] Animation Writer changed to PillowWriter in animate_line and animate_imshow Animation Writer changed to PillowWriter in animate_line and animate_imshow Sep 12, 2019
@rdoyle45

rdoyle45 commented Sep 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Testing complete - Ready for review, then merge @TomNicholas

@rdoyle45

rdoyle45 commented Oct 10, 2019

Copy link
Copy Markdown
Collaborator Author

Can this be merged? @TomNicholas @johnomotani

And then #36 merged into master?

TomNicholas
TomNicholas requested changes Oct 11, 2019
View reviewed changes

@TomNicholas TomNicholas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @rdoyle45 ! Sorry for slow review. If you could just fix the type checking then we can merge :)

Comment thread xbout/tests/test_animate.py Outdated
Comment thread xbout/tests/test_animate.py Outdated
@rdoyle45
Improved use of python functions for pytest and added use of pytest f…
88868d0 
…ixture to create temporary directory to deal with the output of subsequent animate function
@rdoyle45 rdoyle45 requested a review from TomNicholas October 13, 2019 10:52
johnomotani
johnomotani reviewed Oct 14, 2019
View reviewed changes
Comment thread xbout/tests/test_animate.py Outdated
johnomotani
johnomotani approved these changes Oct 14, 2019
View reviewed changes

@johnomotani johnomotani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me 👍
If @TomNicholas is happy, we can merge.

@johnomotani johnomotani merged commit c9d5d2f into boutproject:1d-animations Oct 15, 2019
@johnomotani

johnomotani commented Oct 15, 2019

Copy link
Copy Markdown
Collaborator

I've merged this - if there's anything left to review we can do it in #36

@rdoyle45 rdoyle45 deleted the 1d-animations branch October 15, 2019 21:33
johnomotani added a commit that referenced this pull request Dec 11, 2019
@johnomotani
Merge pull request #52 from rdoyle45/1d-animations
abc82ed 
Animation Writer changed to PillowWriter in animate_line and animate_imshow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnomotani johnomotani johnomotani approved these changes

@TomNicholas TomNicholas Awaiting requested review from TomNicholas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rdoyle45 @pep8speaks @codecov-io @TomNicholas @johnomotani