Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[MRG+1] Fix imshow masked interpolation#8024
Uh oh!
There was an error while loading.Please reload this page.
Conversation
So, defiantly 🐲 in up-sampling when mixing masking + over/under with in the kernel foot print. importmatplotlib.pyplotaspltimportmatplotlibasmplimportmatplotlib.colorsasmcolorsimportmatplotlib.imageasmimageimportmatplotlib.cmasmcmimportnumpyasnpimportcopycm=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]=-1data[7,7]=101data[3,3]=np.nandata[5,3]=np.infmask=np.zeros_like(data).astype('bool')mask[3,5]=Truedata=np.ma.masked_array(data,mask)fig,ax_grid=plt.subplots(3,6)forinterp,axinzip(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 to236b48fCompareOn 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 to5e83b93CompareOf course this passes locally... |
a1fbee5 to73091acCompareWhen determining which pixels to mask in the resampled image, if _any_contribution to final value comes from a masked pixel, mask theresult.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 to1a7c4efCompare| @@ -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 verifyexactly 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 ifremove_text isTrue 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 withremove_text == True
QuLogic commentedFeb 24, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This makes for an ugly change in Cartopy: You might need to view the full resolution image, but it's pretty evident on the lower-most blue one. It used to match the outline, but now there's a very obvious not very straight white bit along the edge. |
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 |
QuLogic commentedMar 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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