FIX patch.update_from to also copy _original_edge/facecolor#10575
FIX patch.update_from to also copy _original_edge/facecolor#10575KonradAdamczyk wants to merge 9 commits into
Conversation
fix for bug described in matplotlib#10574
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
Looks good to me! I should have kept reading before opening the same fix.
|
Actually can you add a small test to check that uodate_property keeps doing the right thing? Need not use legend or have an image test |
jklymak
changed the title
|
Suggest adding to def test_update_from():
# test that update_from properly sets the facecolor.
fig, ax = plt.subplots()
rect = Rectangle((0, 0), 1., 1., facecolor='magenta')
rect2 = Rectangle((1., 0), 1., 1., facecolor='blue')
ax.add_patch(rect)
ax.add_patch(rect2)
rect2.update_from(rect)
rect2.set_alpha(0.8)
assert rect2.get_facecolor() == (1., 0., 1., 0.8)I tried force pushing, but.... |
|
@KonradAdamczyk any chance you can add the test? If not, is it ok to force push onto your PR? |
|
@jklymak |
|
@KonradAdamczyk were you going to add the test, or would you like me to? |
|
@jklymak I already added the test. |
|
It needs to be in the same git branch as this PR |
|
ping @KonradAdamczyk |
|
Great, thanks @KonradAdamczyk ! I'm sure this will attract a second reviewer soon! |
|
The test should go in |
|
Ooops, jeez. Good catch @QuLogic |
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
Please put tests in proper file!
test set_alpha after update_from changes only alpha in color
this test is moved to lib/matplotlib/tests/test_patches.py
| # given | ||
| source_facecolor = (0.234, 0.123, 0.135, 0.322) | ||
| source_egdecolor = (0.728, 0.682, 0.945, 0.268) | ||
| source = mpatches.Rectangle((0, 0), 1., 1., facecolor=source_facecolor, edgecolor=source_egdecolor) |
There was a problem hiding this comment.
Could you make this line and the one below 79 characters or less long by putting some of the arguments on the next line (and lining them up with the brackets?). Otherwise this looks good to go 👍
QuLogic
left a comment
QuLogic
left a comment
There was a problem hiding this comment.
A small typo. You also have some PEP8 errors.
| def test_when_update_from_and_set_alpha_then_only_alpha_changes(): | ||
| # given | ||
| source_facecolor = (0.234, 0.123, 0.135, 0.322) | ||
| source_egdecolor = (0.728, 0.682, 0.945, 0.268) |
|
@dstansby can you approve request now ? |
1 similar comment
|
@dstansby can you approve request now ? |
|
This still has flake8 errors... |
|
I should have just fixed the PEP8 issues, lets see if travis agrees! |
7 participants
fix for bug described in
#10574
PR Summary
PR Checklist