Skip to content
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

BUG: master has broken some 3d plots #2808

Closed
fperez opened this issue Feb 13, 2014 · 12 comments · Fixed by #2851
Closed

BUG: master has broken some 3d plots #2808

fperez opened this issue Feb 13, 2014 · 12 comments · Fixed by #2851

Comments

@fperez
Copy link
Member

fperez commented Feb 13, 2014

This 3d example from the gallery runs fine in 1.3.1, but on master it produces a warning and a completely broken plot:

image

Confirmed on linux by me and OSX by @ellisonbg.

@WeatherGod
Copy link
Member

is it only the tri family of functions that are broken?

@ghost
Copy link

ghost commented Feb 27, 2014

You can fix this issue by :
PolyCollection.init(self, verts, args_, *_kwargs) in art3d.py into
PolyCollection.init(self, verts, **kwargs) OR
PolyCollection.init(self, verts,None ,**kwargs) OR

@pbhandari
Copy link

Hey, here's another fix.

Basically, what I think is happening is: when self._sizes contains a negative number, the np.sqrt doesn't work properly and puts NaNs in the scale array. The fix just checks whether the scale array has any NaNs. If it does, zero out the transforms array, otherwise keep the old behaviour.

The file in question is lib/matplotlib/collections.py.

@@ -720,9 +720,12 @@ class _CollectionWithSizes(Collection):
             self._sizes = np.asarray(sizes)
             self._transforms = np.zeros((len(self._sizes), 3, 3))
             scale = np.sqrt(self._sizes) * dpi / 72.0
-            self._transforms[:, 0, 0] = scale
-            self._transforms[:, 1, 1] = scale
-            self._transforms[:, 2, 2] = 1.0
+            if any(np.isnan(scale)):
+                self._transforms = np.empty((0, 3, 3))
+            else:
+                self._transforms[:, 0, 0] = scale
+                self._transforms[:, 1, 1] = scale
+                self._transforms[:, 2, 2] = 1.0

     def draw(self, renderer):
         self.set_sizes(self._sizes, self.figure.dpi)

@tacaswell
Copy link
Member

@WeatherGod does this look reasonable?

@tacaswell tacaswell added this to the v1.4.0 milestone Feb 27, 2014
@WeatherGod
Copy link
Member

Neither look good to me. The important question to ask is why are NaNs happening at all? Somehow, invalid sizes are getting into Collections. This problem seems to have been introduced with the recent changes to collections. Either mplot3d has been interfacing with PolyCollections incorrectly all these years, or somehow, PolyCollections is allowing invalid values (possibly from defaults?) for sizes.

@sohakes
Copy link

sohakes commented Feb 27, 2014

When I looked at the code, the first impression I had was the same as @limtaesu. I mean, in the art3d.py file, there is a call PolyCollection.__init__(self, verts, *args, **kwargs), but the init function doesn't receive any *args argument (def __init__(self, verts, sizes=None, closed=True, **kwargs)), instead, *args is being passed as the sizes argument to the init function. It looks like the *args was just passed along in case the PolyCollection's init used it, but the wrong argument sizes received it. So at first glance, it looks like it's just a case of confusion in the calls, as no sizes needed to be set at all (at least thats what it looks like, because the default for sizes is None and it worked in the example).

I don't know much neither python or matplotlib, so maybe the *args argument was supposed to store the sizes and I may be wrong. Sorry if it was unhelpful.

@WeatherGod
Copy link
Member

Having *args in the argument list is in case a user passes those arguments as positional arguments. size=None in the PolyCollection constructor is a default argument at that position, so it is both positional and keyword, in a sense. The same is true for the closed=True argument. Put it another way, the following calls are all identical:

PolyCollection(verts, None, True)
PolyCollection(verts, None, closed=True)
PolyCollection(verts, closed=True, sizes=None)
PolyCollection(verts)

@WeatherGod
Copy link
Member

This is really weird... the only way for that particular code path to be executed is if sizes was not None. However, mplot3d and the code example do not supply any additional positional or keyword arguments for sizes, so it should be None. Somewhere, something fishy is happening...

@WeatherGod
Copy link
Member

Ah-ha! I think I found the source of the problem.

Looks like the refactor of the tri* family of functions didn't properly
handle the "z" argument, and it got passed into PolyCollection as a
positional argument. I'll take a closer look at this and put together a PR
today.

On Thu, Feb 27, 2014 at 4:34 PM, sohakes notifications@github.com wrote:

When I looked at the code, the first impression I had was the same as
@limtaesu https://github.com/limtaesu. I mean, in the art3d.py file,
there is a call PolyCollection.init(self, verts, _args, *_kwargs), but
the init function doesn't receive any _args argument (def init(self,
verts, sizes=None, closed=True, *_kwargs)), instead, *args is being passed
as the size argument to the init function. It looks like the *args was just
passed along in case the PolyCollection's init used it, but the wrong
argument (sizes) received it. So at first glance, it looks like it's just a
case of confusion in the calls, as no size needed to be set at all in the
example (at least thats what it looks like, because the default for size is
None and it worked fine).

I don't know much neither python or matplotlib, so maybe the *args
argument was supposed to store the sizes and I may be wrong. Sorry if it
was unhelpful.

Reply to this email directly or view it on GitHubhttps://github.com//issues/2808#issuecomment-36294237
.

@WeatherGod
Copy link
Member

Now, what is probably a more interesting question is why Travis did not catch this. mplot3d has image tests now, and the baseline images are correct.

@tacaswell
Copy link
Member

Make sure that the tests are white listed in tests.py

@WeatherGod
Copy link
Member

Ah, that would explain that. PR is forthcoming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants