Cool!

I suggest returning the whole set of collections added to the axes.

This should probably get a test.

How does this do with alpha in the colors?

attn @WeatherGod

I suggest returning the whole set of collections added to the axes.

• As a list/tuple?

• As a 3-tuple of 3d numpy arrays indexed by location?

  data = np.zeros(10, 20, 30)
  xfaces, yfaces, zfaces = voxels(data)
  assert xfaces.shape == (11,20,30)
  assert yfaces.shape == (10,21,30)
  assert zfaces.shape == (10,20,31)
  

• As a 3d-numpy array of sets indexed by bordering pixel?

This should probably get a test.

Not entirely sure how to do this, especially since I'm unable to pip install matplotlib from source on windows, so can't actually run this code

How does this do with alpha in the colors?

Untested. The machine I tested this on had an old enough version of matplotlib that poly.set_alpha(a) had no effect at all, which was weird.

It doesn't draw internal faces, so a solid cube surrounded by translucent cubes will be invisible. It also becomes hard to decide what color to draw the faces if alpha is involved.

Older versions of mplot3d does not handle alphas correctly, this was fixed
in v1.4.1, IIRC.

I'll look through this in the next few days.

On Thu, May 12, 2016 at 12:41 PM, Eric Wieser notifications@github.com
wrote:

I suggest returning the whole set of collections added to the axes.

• As a list/tuple?

  As a 3-tuple of 3d numpy arrays indexed by location?

  data = np.zeros(10, 20, 30)
  xfaces, yfaces, zfaces = voxels(data)

  assert xfaces.shape == (11,20,30)
  assert yfaces.shape == (10,21,30)
  assert zfaces.shape == (10,20,31)

  As a 3d-numpy array of sets indexed by bordering pixel?

This should probably get a test.

Not entirely sure how to do this, especially since I'm unable to pip
install matplotlib from source on windows, so can't actually run this code

How does this do with alpha in the colors?
Untested. The machine I tested this on had an old enough version of
matplotlib that poly.set_alpha(a) had no effect at all, which was weird.

It doesn't draw internal faces, so a solid cube surrounded by translucent
cubes will be invisible. It also becomes hard to decide what color to draw
the faces if alpha is involved.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6404 (comment)

Behaviour with alphas in a newer matplotlib:

fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')
x, y, z = np.indices((10, 10, 10))
v1 = x == y
v2 = np.abs(x - y) < 2
colors = np.zeros((10, 10, 10, 4))
colors[v2] = [1, 0, 0, 0.5]
colors[v1] = [0, 1, 0, 0.5]
ax.voxels(v1 | v2, colors)
plt.show()

image

Note that the inner green voxels are not rendered, because we only render the surface

Updated to return something

This build failure seems entirely unrelated

2 of them are unreleated and known transient failures see #

However the one that builds the docs is probably related.

/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_zscale:43: WARNING: Inline interpreted text or phrase reference start-string without end-string.

It's most likely because you are missing a ``` after Poly3DCollection

Yep, just caught that too. Fixup'd and repushed

I don't think this error is real?

@WeatherGod, is this still in your queue?

Yeah, it still is in my queue. Just been punting on it as this would have
to go into 2.1, and we are still working on 2.0.

On Tue, Jul 19, 2016 at 3:51 AM, Eric Wieser notifications@github.com
wrote:

@WeatherGod https://github.com/WeatherGod, is this still in your queue?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6404 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-Pu-YTG0UsiX3fYiFsKPZ6DHbMG1ks5qXIH8gaJpZM4Icczh
.

Nice job on the 2.0 release!

Could you give me some direction on how to construct the image tests for this? Do I need to commit the PNGs as well? Do I need a specific rendering backend available for the tests to pass?

I'd still appreciate some tips on finishing this off - it would be great to get it in

Thanks for the review! I'll go through your comments shortly

we need [...] unit tests.

Can you point me to some documentation on how best to do these? In particular, how do I generate the image that the tests compare against? Isn't that platform-dependent?

http://matplotlib.org/devel/index.html

What are the odds of that working on Windows?

@eric-wieser High, we run tests on appevyor 😄 and the same image tests pass on mac, windows, and linux.

The appevyor configuration files may be a good guide to getting a dev enviroment set up with conda.

If filled is required, then put it in the call signature. There's no need for the clever trick.

@QuLogic: Can you show me what you mean? I'm trying to make all of these cases work, as those are the cases allowed by the PEP 457 spec of voxels([x, y, z, ] /, filled=filled, **kwargs)

voxels(filled, **kwargs)
voxels(filled=filled, **kwargs)
voxels(x, y, z, filled, **kwargs)
voxels(x, y, z, filled=filled, **kwargs)

Additionally, this should fail:

voxels(x, y, z, filled, filled=filled2, **kwargs)
# TypeError: voxels() got multiple values for argument 'filled'

The clever trick does a very good job of providing this behaviour, and provides the right error messages on python 3 too. It also makes it super easy to add more both-positional-and-keyword arguments

The behavior you are looking for looks a lot like pcolor(). Perhaps look to that function for how we do that?

Just my 2c; I would prefer all these crazy argument parsing tricks to just use inspect.Signature.{bind,apply_defaults} (yes, that'd need the proper backport to be done).

I would prefer all these crazy argument parsing tricks to just use inspect.Signature

Unfortunately Signature doesn't implement PEP457 yet, so that doesn't help too much. Also, there's no inspect.Signature.from_string method, so we'd need to do something like sig = inspect.signature(lambda x, y, z, *, kwonly: pass). Of course, we can't backport that, because it uses py3 syntax. So now we need to write our own function get_sig("x, y, z, *, kwonly") to implement the py3 grammar!

I do think you're right that a generic argument-parsing helper might be handy, but I don't think that inspect.Signature is up to our needs.

Perhaps look to that function for how we do that?

pcolor does a worse job at processing arguments than voxels (comparing the C and filled arguments) in all of these cases:

pcolor() # TypeError: Illegal arguments to pcolor; see help(pcolor)
voxels() # TypeError: voxels() missing 1 required positional argument: 'filled'
pcolor(arr2d) # ok
voxels(arr3d) # ok
pcolor(C=arr2d) # TypeError: Illegal arguments to pcolor; see help(pcolor)
voxels(filled=arr3d) # ok
pcolor(arr2d, C=arr2d) # AttributeError: Unknown property c
voxels(arr3d, filled=arr3d) #TypeError: voxels() got multiple values for argument 'filled'

Here's a possibly clearer implementation:

 # Use a lambda function to parse out the two forms of arguments.
 # Leading underscores are used to indicate positional-only arguments.
 if len(args) >= 3:
 parser = lambda __x, __y, __z, filled, **kwargs: (
 (__x,__y,__z), filled, kwargs)
 else:
 parser = lambda filled, **kwargs: (
 None, filled, kwargs)
 parser.__name__ = str('voxels') # to avoid unicode_literals
 xyz, filled, kwargs = parser(*args, **kwargs)

Would that be acceptable?

Unfortunately Signature doesn't implement PEP457 yet, so that doesn't help too much. Also, there's no inspect.Signature.from_string method, so we'd need to do something like sig = inspect.signature(lambda x, y, z, *, kwonly: pass). Of course, we can't backport that, because it uses py3 syntax. So now we need to write our own function get_sig("x, y, z, *, kwonly") to implement the py3 grammar!

Actually it nearly does, it's just tucked in a private function (used to parse Argument Clinic signature strings):

In [21]: inspect._signature_fromstr(inspect.Signature, None, "(x, y, z, /, filled=None, **kwargs)", False)
Out[21]: <Signature (x, y, z, /, filled=None, **kwargs)>

(if we don't want to rely on a private function we can just copy-paste the implementation).

Square brackets (optional positional-only arguments) are not implemented (I guess because it's not really possible to represent them as Signature objects anyways), but a possibility would be to provide a list of signatures which would be tried one after the other until one matches.

Oops, you're right. I did know about that, but I assumed it worked by evaling a function with that signature, which in hindsight wouldn't work.

a list of signatures which would be tried one after the other until one matches.

I think I'd be in favor of using Signature here, but I don't think backporting that should hold up this PR. There is a backport in the funcsigs package (https://github.com/aliles/funcsigs), but it looks unmaintained.

That's definitely not a blocker. (I agree it should be a separate PR.)

I've updated the implementation to use a form similar to the above comment. It fails, but only because of docs.scipy.org being inaccessible.

Actually I think I would still favor something like

parsers = []
@parsers.append
def voxels(x, y, z, filled, **kwargs): return ...
@parsers.append
def voxels(filled, **kwargs): return ...
for parser in parsers:
 try: args, kwargs = parser(*args, **kwargs)
 except TypeError: pass
 else: break
else: raise TypeError("no signatures match") # some appropriate error message.

(but not a big deal either)

@anntzer: That approach makes it difficult to produce useful error messages.

Actually it's the opposite: Python is not able to autogenerate error messages for such signature-overloaded functions, so it should be better to just provide the error message ourselves.

Sorry, I did not intend there to be such a long detour on what was ultimately a misunderstanding on my part wrt whether filled is needed.

On to something I did understand though, PEP457 may be a standard, but I'm not so sure we can rely on users having read it (I certainly haven't until now.) So I think you will have to list out all the individual ways of calling the function (as we do in, e.g., pcolor), regardless of what the implementation ends up being.

So I think you will have to list out all the individual ways of calling the function

Done, along with some other fixes to the parameter documentation

Thanks @eric-wieser !

This is a pretty cool feature to get in 😄

Pleased to see it finally made it! Thanks everyone for the feedback :)