FIX: Correct variable name from _frame to _frames in PillowWriter class #29520
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
@rcomer can you take a look at it later, please? |
| @@ -526,3 +526,31 @@ def test_movie_writer_invalid_path(anim): | |||
| with pytest.raises(FileNotFoundError, match=match_str): | |||
| anim.save("/foo/bar/aardvark/thiscannotreallyexist.mp4", | |||
| writer=animation.FFMpegFileWriter()) | |||
|
|
|||
|
|
|||
| def test_exhausted_animation_with_transparency(tmp_path): | |||
There was a problem hiding this comment.
What exactly is the purpose of the test?
From a quick reading it is
- a smoke test to exercise the anim code path for transparency
- testing that a saved animation is exhausted.
Can you please describe more precisely what the test wants to ensure? Also it seems (2) is quite unrelated to transparency.
There was a problem hiding this comment.
The goal of this test is to actually verify the non-covered section:
if im.getextrema()[3][0] < 255:
# This frame has transparency, so we'll just add it as is.
self._frames.append(im)via savefig_kwargs={"transparent": True}.
I tried just to mock up the class PillowWriter(AbstractMovieWriter):, but it actually raise an error that savefig_kwargs={"transparent": True} is not included. Consequently, I extended a little bit the test. Considering, to leave this part out?
# Verify exhausted warning
with pytest.warns(UserWarning, match="exhausted"):
anim._start()There was a problem hiding this comment.
@timhoffm what about this test? Only using the PillowWriter?
def test_animation_with_transparency():
"""Test animation exhaustion with transparency using PillowWriter directly"""
fig, ax = plt.subplots()
rect = plt.Rectangle((0, 0), 1, 1, color='red', alpha=0.5)
ax.add_patch(rect)
ax.set_xlim(0, 1)
ax.set_ylim(0, 1)
writer = PillowWriter(fps=30)
writer.setup(fig, 'unused.gif', dpi=100)
writer.grab_frame(transparent=True)
frame = writer._frames[-1]
# Check that the alpha channel is not 255, so frame has transparency
assert frame.getextrema()[3][0] < 255
plt.close(fig)so the missing part is also captured? thx for feedback
There was a problem hiding this comment.
If former test fits better, I will submit a revert commit
…to _frames in PillowWriter class
…520-on-v3.10.x Backport PR #29520 on branch v3.10.x (FIX: Correct variable name from _frame to _frames in PillowWriter class)
PR summary
Correct the variable name from
_frameto_framesin the PillowWriter class to ensure proper functionality.PR checklist