-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
[MRG+1] Fix imshow masked interpolation #8024
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.
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
[MRG+1] Fix imshow masked interpolation #8024
Conversation
So, defiantly 🐲 in up-sampling when mixing masking + over/under with in the kernel foot print. import matplotlib.pyplot as plt
import matplotlib as mpl
import matplotlib.colors as mcolors
import matplotlib.image as mimage
import matplotlib.cm as mcm
import numpy as np
import copy
cm = copy.copy(mcm.get_cmap('viridis'))
cm.set_over('r')
cm.set_under('b')
cm.set_bad('k')
n = mcolors.Normalize(vmin=0, vmax=100)
data = np.arange(100, dtype='float').reshape(10, 10)
data[5, 5] = -1
data[7, 7] = 101
data[3, 3] = np.nan
data[5, 3] = np.inf
mask = np.zeros_like(data).astype('bool')
mask[3, 5] = True
data = np.ma.masked_array(data, mask)
fig, ax_grid = plt.subplots(3, 6)
for interp, ax in zip(mimage._interpd_, ax_grid.ravel()):
ax.set_title(interp)
im = ax.imshow(data, norm=n, cmap=cm, interpolation=interp)
|
Right, forget about the topo failure, that is due to the resample vs not resample change and I have not looked into it yet. |
8786852
to
236b48f
Compare
On the bright side I have managed to fix this without having to change classic style and only update 1 test image, on the down side, one test is still broken. |
and I have fixed the checker-board effect in that png above, that was due to Agg's aggressive clipping. |
236b48f
to
5e83b93
Compare
Of course this passes locally... |
a1fbee5
to
73091ac
Compare
When determining which pixels to mask in the resampled image, if _any_ contribution to final value comes from a masked pixel, mask the result. Due to Agg special-casing the meaning of the alpha channel, the interpolation for the mask channel needs to be done separately. This is probably a template for doing the over/under separately. print out exact hash on travis
73091ac
to
1a7c4ef
Compare
@@ -76,6 +76,7 @@ before_install: | |||
|
|||
install: | |||
- ccache -s | |||
- git describe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not concerned about this, but would like to make sure that you intended to have this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, was a bit worried that gh/travis were having caching / timeout issues and not running the code I thought it was running (turns out the problem is I had accidentally depended on dictionary ordering in 3.6). I think this is a good idea to keep around so that we can verify exactly what code travis runs for testing locally (it should be running on the merge into the target branch, when it re-sets what that merge is is not something I fully understand yet).
The CI failure appears non-transient. |
What happened to Appveyor? |
It doesn't run on the |
How did I not know that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me. Only a minor question.
# 'unshare' the mask array to | ||
# needed to suppress numpy warning | ||
del out_mask | ||
invalid_mask = ~output.mask * ~np.isnan(output.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would &
work here instead of *
? Would make more sense given the boolean masks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there performance rather than semantic advantages? I think ~(a & b)
would also work and have one less temprorary.
|
||
fig, ax_grid = plt.subplots(3, 6) | ||
for interp, ax in zip(sorted(mimage._interpd_), ax_grid.ravel()): | ||
ax.set_title(interp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any point adding a title if remove_text
is True
in the decorator. Otherwise this patch looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title is there for human debuggers later (I often copy-past tests into another buffer and just run them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
for interp, ax in zip(sorted(mimage._interpd_), ax_grid.ravel()): | ||
ax.set_title(interp) | ||
ax.imshow(data, norm=n, cmap=cm, interpolation=interp) | ||
ax.axis('off') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, prseumably pointless with remove_text == True
sigh |
The build also failed |
Even weirder, #8144 is failling on that test on 3.5 but not other versions, even after a rebuild... |
The rotate image test did give me trouble when working on this. |
I have sorted out the reason (but not the cause) of the rotate_image failures, for some reason the |
Maybe it should use |
That would fix it, but we should also fix |
Closes #8012
The code changes are in the first commit, the image changes are in the second.
This probably needs some more tests and docs. I suspect there is at least one more 🐉 down here...
attn @mdboom