Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Handle NaN values inplot_surface zsort#20646
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
jklymak commentedJul 20, 2021
@eric-wieser@WeatherGod did you have any comments on whether this is preferred to#18114? |
tomneep commentedJul 21, 2021
@jklymak just to be sure there is no confusion, I think this is potentially useful regardless of#18114. The relationship with#18114 is that if this PR is merged, then we can solve the masking issue in a much simpler way that what I did in#18114. But if it decided that you don't want to support NaNs in |
lib/mpl_toolkits/mplot3d/art3d.py Outdated
| defnansafe(func): | ||
| deff(x): | ||
| value=func(x) | ||
| returnnp.infifnp.isnan(value)elsevalue |
eric-wieserJul 21, 2021 • 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.
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.
Is this actually correct behavior? Would it be better to remove the NaNs before computing the mean /min / max / whatever?
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.
Hmmm yes I think we need both. I was just looking at plots where polys containing NaNs are completely masked, but in cases where you have just one masked corner the rendering is still wrong. One way to solve this would be to replace the_zsort_functions with theirnan equivalents and then work around theRuntimeWarning raised if all elements of the tested array areNaN.
So
matplotlib/lib/mpl_toolkits/mplot3d/art3d.py
Lines 720 to 724 in498bcdd
| _zsort_functions= { | |
| 'average':np.average, | |
| 'min':np.min, | |
| 'max':np.max, | |
| } |
would be replaced with
_zsort_functions= {'average':np.nanmean,# mean and average the same with unweighted data?'min':np.nanmin,'max':np.nanmax, }
and then we'd rewrite thenansafe wrapper (or the method like you suggest) to be
defnansafe(func):deff(x):returnnp.infifnp.isnan(x).all()elsefunc(x)returnf
Do you think that would be a solution? Do you see any problem with replacing the functions inside _zsort_functions?
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 haven't had time to look into the details here. But keep in mind that the sorting is done everytime you rotate the view, which we want to be a real-time response. No question that filtering nans is necessary for correctness, but if you have multiple ways of doing that, please evaluate the performance.
| self._sort_zpos=None | ||
| self.stale=True | ||
| def_zsortval(self,zs): |
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.
| def_zsortval(self,zs): | |
| def_zsortval(self,zs): | |
| """Computethevaluetouseforz-sortinggiventheviewerzcoordinatesofanobject`zs`,with | |
| largervaluesdrawnunderneathsmallervalues. | |
| Thisfunctionshouldneverreturn`nan`,andreturns`np.inf`ifnonon-nanvalueiscomputable. | |
| """ |
tomneep commentedJul 22, 2021
In response to thecomment from@timhoffm I had a quick look at performance in a rather unscientific manner (please let me know if you want something better). I adapted the example from#12395 to perform a full rotation and looked at how long it took. Test codeimportnumpyasnpimportmatplotlib.pyplotasplt# Generate the datax=np.linspace(-1.0,1.0,50)y=np.linspace(-1.0,1.0,50)x,y=np.meshgrid(x,y)z= (1-y/x).clip(min=-5.0,max=5.0)# place NaNs at the discontinuitypos=np.where(np.abs(np.diff(z))>=5.0)z[pos]=np.nan# Create the plotax=plt.figure().add_subplot(projection="3d")surf=ax.plot_surface(x,y,z,rstride=1,cstride=1,cmap="coolwarm",linewidth=0,vmin=np.nanmin(z),vmax=np.nanmax(z),antialiased=False,)ax.set_title("1 - y/x")ax.set(xlim=(-1,1),ylim=(-1,1),zlim=(-5,5))ax.set(xlabel="x",ylabel="y",zlabel="z")# rotate the axes and updateforangleinrange(0,360):ax.view_init(30,angle)plt.draw()plt.pause(0.001) Wall times:
I also tried using the with I ran these a few times and the results were pretty consistent, although I realise this is a pretty rubbish way to test this kind of thing. |
jklymak commentedJul 22, 2021
Can you cache the NaN's somehow? I've not traced this code carefully, but is there any reason to keep NaNs in the list of vertices and colors etc? |
eric-wieser commentedJul 22, 2021
Even caching a mask of where the nans are would probably help. |
tomneep commentedJul 22, 2021
@jklymak Yeah I think this can be solved by just stripping any NaNs before making the PolyCollection3D. I actually have another branch that does exactly that in What would be the preferred way forward? To open a new PR (third time lucky!) or force push here? |
jklymak commentedJul 22, 2021
Whatever is easiest for you, but if you opened new then make sure to include relevant comparisons |
tomneep commentedJul 22, 2021
Ok thanks, I've converted this to a draft while I sort everything out. I'll make sure to clearly include before and after comparisons of the changes. Thanks again for all the comments so far. |
QuLogic commentedAug 12, 2021
Replaced by#20725, I believe. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Closes#8222#12395
While investigating different ways of solving#18114 I came across the issue where NaNs in data causing strange plotting issues. I tracked this down to the sorting of polygons where the python builtin
sortedmethod is used. This doesn’t deal with NaNs so we need to be sure to make the zsort functions NaN safe. This does that by checking if the function returns NaN and if so returningsys.maxsize.I want to separate this from#18114 as I found two open issues I think it solves,#8222 and#12395.
plot_surfacedoes warn that it can't deal with NaNs but it looks like that was added in response to#12395. This seems to solve that, so perhaps the warning can be removed?I still need to add some tests but I wanted to check there was interest in adding this before writing those. It seems like a fairly simple problem to solve so maybe there is some deeper reason this hasn't been done?
The before and after plots of the two issues are
#8222 Before
#8222 After
#12395 Before
#12395 After
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).