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.

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

Animation Writer changed to PillowWriter in animate_line and animate_imshow #52

Merged
merged 16 commits into from
Oct 15, 2019
Merged

Animation Writer changed to PillowWriter in animate_line and animate_imshow #52

merged 16 commits into from
Oct 15, 2019

Conversation

rdoyle45
Copy link
Collaborator

@rdoyle45 rdoyle45 commented Sep 11, 2019 •
edited
Loading

Writer change requested by @TomNicholas in pull request #36

@pep8speaks
Copy link

pep8speaks commented Sep 11, 2019 •
edited
Loading

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
Copy link

codecov-io commented Sep 11, 2019 •
edited
Loading

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
xbout/plotting/animate.py Outdated Show resolved Hide resolved
@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
xbout/plotting/animate.py Outdated Show resolved Hide resolved
@TomNicholas
Copy link
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
Copy link
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
Copy link
Collaborator Author

rdoyle45 commented Sep 12, 2019 •
edited
Loading

Testing complete - Ready for review, then merge @TomNicholas

@rdoyle45
Copy link
Collaborator Author

Can this be merged? @TomNicholas @johnomotani

And then #36 merged into master?

TomNicholas
TomNicholas requested changes Oct 11, 2019
View reviewed changes
Copy link
Collaborator

@TomNicholas TomNicholas left a comment

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 :)

xbout/tests/test_animate.py Outdated Show resolved Hide resolved
xbout/tests/test_animate.py Outdated Show resolved Hide resolved
@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
johnomotani
johnomotani approved these changes Oct 14, 2019
View reviewed changes
Copy link
Collaborator

@johnomotani johnomotani left a comment

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
Copy link
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