GeezerVison

I think breaking the return type is a tough ask, though I'm not sure how to better structure it.

No worries on refreshing the comparison images, it's not too many. Though to my eye it seems like voxels-scalar-xyz.png and voxels-scalar.png are the same? Could potentially drop one of them and compare against a single image.

I think breaking the return type is a tough ask, though I'm not sure how to better structure it.

#30733 technically did that for pie and my guess is pie is probably used a lot more than voxels. I think the big thing there was making sure that the return object was still interable, not sure if there's an equivalent here.

@story645 @scottshambaugh Thank you for the feedback, please let me know what you think of the following proposition :)

One possibility is to change the return type of ax.voxel() to a subclass of dict, but which can also act as a mappable for a colorbar:

class VoxelDict(dict):
    """
    A dictionary subclass indexed by coordinate, where ``faces[i, j, k]``
    is a `.Poly3DCollection` of the faces drawn for the voxel
    ``filled[i, j, k]``. If no faces were drawn for a given voxel,
    either because it was not asked to be drawn, or it is fully
    occluded, then ``(i, j, k) not in faces``.

    This class also supports the functionality required to act as a mappable
    for a colorbar.
    """

    def __init__(self, axes, colorizer):
        """
        Parameters
        ----------
        axes : `mplot3d.axes3d.Axes3D`
            The axes the voxels are contained in.

        colorizer : `mpl.colorizer.Colorizer`
            The colorizer uset to convert data to color.
        """
        super().__init__(self)
        self.axes = axes
        self.colorizer = colorizer
        self._callbacks = cbook.CallbackRegistry(signals=["changed"])
        self._A = None

I made a new branch with the required changes in a single commit here: main...trygvrad:matplotlib:voxels_3d_colorizer_v2

The class needs to have the following functions:

    def cmap(self):
    def norm(self):
    def callbacks(self):
    def get_array(self):
    def set_array(self, A):
    def add_callback(self, func):
    def remove_callback(self, oid):
    def changed(self, *args):
    def get_alpha(self):
    def autoscale(self, A):
    def autoscale_None(self):

where we have an additional layer of callbacks to connect the voxels [Poly3DCollections] to the colorbar.

As an alternative I tried to make the new return type also inherit from ColorizingArtist, i.e.

class VoxelDict(dict, ColorizingArtist):
    ...

I was able to make this work, and this simplified the class, but I observed much decreased performance. I think something was wrong with the callback. Ultimately, I found that the architecture is easier to read if the new class does not subclass from ColorizingArtist

The class needs to have the following functions:

If we are not subclassing ColorizingArtist, we should at least have a Protocol that defines what something colorizable must implement.

One possibility is to change the return type of ax.voxel() to a subclass of dict [...]

It's typically not a good idea to inherit from dict. For our purposes, it should be enough to inherit from collections.abc.Mapping.

Generally, we are aspiring to move to semantic Artists that capture the logic structure of the represented data, not just a bunch of basic artists that can be used to draw the data. For example in the pie() case mentioned above, the result had been just a bunch of wedges and texts. Once created, it's basically impossible to update the data values, because number, position and size of the wedges non-trivially depends on data values. So updating data will only be possible when we have an Artist that knows how the data is translated into basic Artists.

Voxels are in a similar situation: They can only be colored as a single collection, but are currently crated as multiple Poly3dColletions. Here, just putting them all in one Poly3dCollection is still not good enough, because that does not track the individual voxel, so a hard-coded set_facecolors([voxel1_color, ..., voxeln_color]) is still not possible.

This is just to give direction, it hasn't to be done all in one step. PieContainer is currently also a very shallow container, but is designed to be extendable in the above described direction.

On an additional note, composition or inheritance can both work for the semantic Artists. PieContainer uses composition, StepPatch in contrast was created as a semantic Artist for stairs() and uses inheritance.

Thank you @timhoffm

Just a quick question, I'm not quite sure what you mean by:

Voxels are in a similar situation: They can only be colored as a single collection, but are currently crated as multiple Poly3dColletions.

They are created as multiple Poly3dColletions, but since they share a single colorizer, we can update the data either on the Poly3dColletions themselves, or via a helper function in the VoxelData type:

    def set_array(self, A):
        """
        Set the value array from array-like *A*.

        Parameters
        ----------
        A : array-like of length equal to the number of voxels.
            The values that are mapped to colors.

        """

        self._A = A
        for a, k in zip(A, self.keys()):
            self[k].set_array(a)

We could easily do the same for set_facecolor(), which would allow the syntax set_facecolors([voxel1_color, ..., voxeln_color]).

I'll take a look the other feedback you provided later :)
Perhaps we would also like to have an artist where we can update which voxels are rendered after creation...

My bad, I was still thinking in the ScalarMappable world where you couldn't reasonably share the colorizing config across multiple artists.

Closing this as I have opened #31032 instead