GeezerVison is showing github.com — consider descriptor pattern for `GraphicFeature`. · Issue #389 · fastplotlib/fastplotlib
consider descriptor pattern for GraphicFeature. #389DescriptionWhile playing around with some examples, I saw this type error reported by mypy:
I dug into a bit and learned about the Graphic/GraphicFeature relationship. Apologies if you already know this and specifically decided against it... but the general pattern of an abstract _set method on GraphicFeature objects that retain a pointer to some parent object looks very much like it could be replaced/improved with python descriptors.
For example, the current pattern (highly simplified) is:
class GraphicFeature:
def __init__(self, parent):
self._parent = weakref.proxy(parent)
def _set(self, value):
raise NotImplementedError
class SomeFeature(GraphicFeature):
def _set(self, value):
print(f"setting {self.name!r} on {self._parent} to {value!r}")
class Graphic:
def __setattr__(self, key, value):
if hasattr(self, key):
attr = getattr(self, key)
if isinstance(attr, GraphicFeature):
attr._set(value)
return
super().__setattr__(key, value)
class SomeGraphic(Graphic):
def __init__(self) -> None:
self.feature = SomeFeature()with the descriptor pattern, that all reduces to
class SomeFeature:
def __get__(self, instance, owner):
...
def __set__(self, obj, value):
print(f"setting feature on {obj} to {value!r}")
class SomeGraphic:
feature = SomeFeature()and when you set the feature attribute on an instance of SomeGraphic, then SomeFeature.__set__ will be called with the instance and value:
In [18]: g = SomeGraphic()
In [19]: g.feature = 1
setting feature on <__main__.SomeGraphic object at 0x106c33750> to 1 this also works very nicely for typing:
note that it neither complains when setting .feature = [1,2,3], and it also recognizes that accessing .feature will return a str (i.e. it's been coerced from list to some internal value), based on the type hints of __set__ and __get__ Activitykushalkolar commented on Dec 4, 2023 Thanks for all this!! I will have to dig into my old PRs in a few hours to see why we didn't do this, but it's likely I didn't know it could be done like this.
We don't use any getters for GraphicFeatures, they have a few methods on them which are public such as "add_event_handler()", and the "values" property for CmapFeature.
graphic.colors.add_event_handler
So it should be sufficient to just implement __set__ kushalkolar commented on Dec 5, 2023 I looked into descriptors and our own issues and this would generally be within the scope of a GraphicFeatures refactor that we want to do sometime next year in conjuntion with refactoring feature events, probably using observ
By using __set__ as you suggested we can do something like this, if you have ideas on how to improve this they would be welcome!
class SomeFeature:
def __init__(self, value):
self._value = value
self._handlers = list()
def __set__(self, obj, value):
print(f"setting feature on {obj} to {value!r}")
self._call_handlers({"value": value})
def __call__(self):
return self._value
def add_event_handler(self, func: callable):
self._handlers.append(func)
def _call_handlers(self, ev: dict):
for f in self._handlers:
f(ev)
class SomeGraphic:
feature = SomeFeature("foo")>>> g = SomeGraphic()
>>> g.feature
<__main__.SomeFeature at 0x7ff4681baad0>
>>> g.feature()
'foo'
>>> g.feature.add_event_handler(lambda ev: print(f"event happened: {ev}"))
>>> g.feature = "bar"
setting feature on <__main__.SomeGraphic object at 0x7ff4681c44d0> to 'bar'
event happened: {'value': 'bar'}If you're interested in helping out let us know 😄 tlambert03 commented on Dec 5, 2023 ContributorAuthor More actions
refactor that we want to do sometime next year in conjuntion with refactoring feature events, probably using observ
hah, yes I was also going to bring up psygnal with you at some point, which is also an observer pattern that arose from our needs of having an event layer at napari outside of vispy. It's rather easy to get into trouble with strong-references to object-bound callbacks, and that lib tries very hard to be careful with both references and types. thanks for pointing out observ, i'll have a look
yeah I'm happy to discuss this stuff more. Not sure if I'll have time to be a part of the implementing. A quick note on your pattern above. If you want any of those other methods to be instance specific, you will want to add a __get__ method, and it should return an object (bound to the instance) that has all the events, and can retrieve values. and the descriptor object itself shouldn't have a _value (since it will be shared across all instances of SomeGraphic in your example above).
In [14]: g1 = SomeGraphic()
In [15]: g2 = SomeGraphic()
In [16]: g1.feature.add_event_handler(print)
In [17]: g2.feature._handlers
Out[17]: [<function print at 0x104cff240>] so, there's more to be discussed about the descriptor pattern... kushalkolar commented on Dec 6, 2023 If the descriptors are instance attributes rather than class attributes that should be fine right?
I think it would be useful to get your thoughts if you're free to chat briefly in early January? tlambert03 commented on Dec 6, 2023 ContributorAuthor More actions
I think it would be useful to get your thoughts if you're free to chat briefly in early January?
yep, happy to chat then
If the descriptors are instance attributes rather than class attributes that should be fine right?
descriptors are, by definition, class attributes (not instance attributes) that have special properties when accessed or set, that give them access to the class and/or instance on which they are being accessed/set. more details in the technical tutorial ... but note how, in my last example above, when you call add_event_handler on g1.feature, it actually also adds it to g2.feature._handlers. Now, it doesn't have to be that way, (it's essentially a bug in the way you've implemented it above), but it does demonstrate that you have to be a bit careful, and understand exactly what the descriptor protocol implies for attribute access. almarklein commented on Mar 13, 2024 For what it's worth, I was a bit surprised by the behavior of features:
>>> sinc_graphic.thickness
ThicknessFeature for LineGraphic @ 0x161eba050, call `<graphic>.thickness()` to get value
>>> sinc_graphic.thickness()
5
>>> sinc_graphic.thickness = 3 I think most people expect a certain symmetry between the setter and getter, e.g.
line_graphic.thickness = other_line_graphic.thickness So although I appreciate the idea to add a handler to the feature like that, it results in, IMO, a somewhat whimsical API that may confuse users a bit.
An alternative way to add a handler might be:
>>> sinc_graphic.add_handler("thickness", my_handler)
Anyway, just my 2ct kushalkolar commented on Mar 13, 2024 Yes I agree I'm not the happiest about how we handle this and the events. Can discuss it, potentially next Wednesday. I became very used to Qt's way of doing things, valueChanged.connect(). I eventually want to look into observ as well which was recommended by Ivo. There's also psygnal by Tally which is modelled directly after Qt.
Could also consider traitlets which uses the descriptor pattern. (and rename to graphic traits if we do that?) kushalkolar commented on Mar 28, 2024 I just tried this and the descriptor pattern doesn't give us what we need, i.e. __setitem__ and __getitem__ functionality as far as I know, I will explore this more:
def parse_colors(value, n):
if isinstance(value, str):
return np.array([pygfx.Color(value)] * n)
class ColorFeature:
def _get_buffer(self, instance) -> pygfx.Buffer:
return instance.world_object.geometry.colors
def __get__(self, instance, owner):
return self._get_buffer(instance).data
def __set__(self, obj, value):
# __set__ and __get__ work but we need slicing and fancy indexing
buffer = self._get_buffer(obj)
buffer.data[:] = parse_colors(value, obj._n_vertices)
buffer.update_range()
self._changed(value)
def __setitem__(self, *args):
# setitem and getitem don't do anything in descriptors
print(args)
# buffer = self.get_buffer(obj)
# buffer[key] = self.parse_colors(value, key.stop - key.start)
def __getitem__(self, *args):
print(args)
def _changed(self, value):
print(f"new val: {value}")
class Graphic:
colors = ColorFeature()
def __init__(self, colors="r"):
xs = np.linspace(0, np.pi * 2, 100)
ys = np.sin(xs)
zs = np.zeros(xs.size)
self._n_vertices = xs.size
self.data = np.column_stack([xs, ys, zs]).astype(np.float32)
colors = parse_colors(colors, self._n_vertices)
self.world_object = pygfx.Line(
geometry=pygfx.Geometry(positions=self.data, colors=colors),
material=pygfx.LineMaterial(thickness=5, color_mode="vertex")
)In the worst case if this isn't possible I will refactor GraphicFeatures so that there's at least symetry between the setter and getter. almarklein commented on Mar 28, 2024
I just tried this and the descriptor pattern doesn't give us what we need, i.e. setitem and getitem functionality as far as I know,
Aah, I see what you mean. I wonder if its at all possible to realize a getter then. Because:
graphic.some_feature[10:20] = 10
/\
getter is invoked heretlambert03 commented on Mar 28, 2024 ContributorAuthor More actions That's not quite how descriptors work. Don't think of __get__ as returning the data itself; think of it as returning "what you get when you access this attribute"
So, if you want something like:
obj = Graphic()
obj.colors[index] = ... Then you need ColorFeature.__get__ to return an object that supports __setitem__. Does that make sense? The descriptor pattern is definitely flexible enough to support what you want here, it's just that it's not the descriptor itself that needs to have the get/setitem methods. almarklein commented on Mar 28, 2024 IIUC the problem is that when ColorFeature.__get__returns a special object as to support indexing, this means that a user simply getting the data does not get the familar data (numpy array, I think) but a special object instead.
Not necessarily the end of the road, but then you'll need to think about an object that supports the buffer protocol so that it can easily be converted into an array. Or subclassing ndarray. Nicolas Rougier made something similar to autosync arrays to the GPU here. tlambert03 commented on Mar 28, 2024 ContributorAuthor More actions Yes, if you want obj.colors itself to be a numpy array, then you'd have to return a numpy array from __get__. I don't remember all the details of the use case here to know whether that's a problem :) tlambert03 commented on Mar 28, 2024 ContributorAuthor More actions if the issue is that you're trying to have the user-facing object be a numpy array, but you also want to know whether they've updated that object... then you've got the legitimately difficult design issue that was faced in napari, where it impossible for napari to know whether a user had done in-place mutations on layer.data, because it was decided that the user object would be the numpy array. If that's the issue you're facing here, you could try psygnal.containers.EventedObjectProxy which was designed for this sort of use case: exposing an object that is as-similar as possible to the interface your users are familiar with, but which lets you know about all mutation events: https://psygnal.readthedocs.io/en/latest/API/proxy/ almarklein commented on Mar 29, 2024 Now that I understand this better, the sinc_graphic.thickness() does not look so bad after-all 😆 kushalkolar commented on Mar 29, 2024
Not necessarily the end of the road, but then you'll need to think about an object that supports the buffer protocol so that it can easily be converted into an array. Or subclassing ndarray. Nicolas Rougier made something similar to autosync arrays to the GPU here.
Been thinking about this a bit and yes I think this would be the way to do it if GraphicFeatures were descriptors, but it adds a layer of complexity which doesn't add additional functionality if I'm understanding this correctly.
if the issue is that you're trying to have the user-facing object be a numpy array, but you also want to know whether they've updated that object... then you've got the legitimately difficult design issue that was faced in napari, where it impossible for napari to know whether a user had done in-place mutations on layer.data, because it was decided that the user object would be the numpy array. If that's the issue you're facing here, you could try psygnal.containers.EventedObjectProxy which was designed for this sort of use case: exposing an object that is as-similar as possible to the interface your users are familiar with, but which lets you know about all mutation events: https://psygnal.readthedocs.io/en/latest/API/proxy/
Thanks for the suggestion! I think that similar to the GPUData array mentioned above it adds more complexity.
I'm going to dig in and play with this over the next few days to try and maximize functionality while minizing API awkwardness 😆 kushalkolar commented on Mar 30, 2024 Been digging into this more and it's really bothering me 😂 . I think the way forward is something like the GPUData array.
My option is to either implement another layer of wrappers, or submit a PEP to change Python behavior 😆
Edit: yea the roadblock is that descriptors don't have a __setitem__ and just having __get__ is insufficient in the descriptors to modify arrays in-place and know of the change. kushalkolar commented on Mar 31, 2024 Here's what I've come up with that maybe makes things less awkward while also letting us keep track of in-place buffer data modifications. Having the GraphicFeatures in @properties helps specify the types for getting and setting so that helps? And also an explicit feature.data property instead of the call, i.e. feature(). Thoughts @almarklein ?
import numpy as np
import pygfx
class BufferManager:
"""Small wrapper for pygfx.Buffer"""
def __init__(self, data, isolated_buffer):
if isolated_buffer:
# useful if data is read-only, ex. memmaps
bdata = np.zeros(data.shape, dtype=data.dtype)
else:
# use the user's array as the buffer
bdata = data
self.buffer = pygfx.Buffer(bdata)
@property
def data(self):
return self.buffer.data
def __getitem__(self, key):
return self.buffer.data[key]
def __setitem__(self, key, value):
pass
def _update_range(self, start, size):
print(f"update range: {start}, {size}")
self.buffer.update_range(offset=start, size=size)
def __repr__(self):
return f"{self.__class__.__name__} buffer data:\n"\
f"{self.data.__repr__()}"
def cleanup_slice(self, key):
"""cleanup slice indices for setitem"""
start = key.start if key.start is not None else 0
stop = key.stop if key.stop is not None else self.data.shape[0]
step = key.step if key.step is not None else 1
return range(start, stop, step)
def parse_colors(value, n):
"""parse colors using pygfx and return RGBA array for each vertex"""
if isinstance(value, str):
return np.array([pygfx.Color(value)] * n)
return value
class ColorFeature(BufferManager):
"""Manage color buffer for positions type objects"""
def __init__(self, data: str | np.ndarray, n_colors: int, isolated_buffer: bool):
if not isinstance(data, np.ndarray):
# isolated buffer is only useful when data is a numpy array
isolated_buffer = False
colors = parse_colors(data, n_colors)
super().__init__(colors, isolated_buffer)
def __setitem__(self, key, value):
if isinstance(value, BufferManager):
# trying to set feature from another feature instance
value = value.data
key = self.cleanup_slice(key)
colors = parse_colors(value, len(key))
self.buffer.data[key] = colors
self._update_range(key.start, key.stop - key.start)
class Graphic:
@property
def colors(self) -> ColorFeature:
"""color feature"""
return self._colors
@colors.setter
def colors(self, colors: str | np.ndarray | ColorFeature):
"""set colors"""
self._colors[:] = colors
def __init__(self, data: np.ndarray, colors: str | np.ndarray | ColorFeature):
n_vertices = data.shape[0]
if isinstance(colors, ColorFeature):
# allow sharing buffers between world objects, just an idea for now
self._colors = colors
else:
self._colors = ColorFeature(data=colors, n_colors=n_vertices, isolated_buffer=False)
self.world_object = pygfx.Line(
geometry=pygfx.Geometry(positions=data, colors=self._colors.buffer),
material=pygfx.LineMaterial(thickness=20, color_mode="vertex")
)Functionality:
xs = np.linspace(0, np.pi * 2, 10)
ys = np.sin(xs)
zs = np.zeros(xs.size)
data = np.column_stack([xs, ys, zs]).astype(np.float32)
g = Graphic(data=data, colors="r")
pygfx.show(g.world_object) 
The repr prints the array:
>>> g.colors
ColorFeature buffer data:
array([[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.]], dtype=float32)returns slice of buffer from ColorFeature.__getitem__
>>> g.colors[5:10]
array([[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.],
[1., 0., 0., 1.]], dtype=float32)Set value to a slice:
>>> g.colors[2:7] = "b"
update range: start: 2, size: 5 
Set to entire line:
>>> g.colors = "y"
update range: start: 0, size: 10 
Make another graphic to set colors from one graphic to another:
ys = np.cos(xs)
zs = np.zeros(xs.size)
data2 = np.column_stack([xs, ys, zs]).astype(np.float32)
g2 = Graphic(data=data2, colors="g")
pygfx.show(g2.world_object) 
>>> g2.colors = g.colors
update range: start: 0, size: 10 
almarklein commented on Apr 2, 2024 I think you've found a pretty good sweet spot!
You can also use a descriptor so you can write the @property stuff using one line colors = ColorFeatureDescriptor, and that descriptor can return the ColorFeature instance.
Idea: could also add a stub array interface to help users. They'd get a helpful error when they tried to use the buffer as an array. On the other hand, I'm not sure if there's code somewhere that checks for the presence of __array_interface__, and providing one that raises an error might be harmful ...
import numpy as np
class Foo:
@property
def __array_interface__(self):
name = getattr(self, "name", "feature")
raise RuntimeError(f"Cannot use a Feature as an array. Use {name}.data instead.")
f = Foo()
a = np.array([1,2,3])
a + fGets:
RuntimeError: Cannot use a Feature as an array. Use feature.data instead.
kushalkolar commented on Apr 2, 2024
I think you've found a pretty good sweet spot!
You can also use a descriptor so you can write the @property stuff using one line colors = ColorFeatureDescriptor, and that descriptor can return the ColorFeature instance.
Great to have a comprose, I think I'll do this refactor later in the week!
Idea: could also add a stub array interface to help users. They'd get a helpful error when they tried to use the buffer as an array. On the other hand, I'm not sure if there's code somewhere that checks for the presence of __array_interface__, and providing one that raises an error might be harmful ...
Ah good idea! I think that this will work though:
image.data = image.data + np.random.rand(image.data[:].shape)
/\ slicing will invoke __getitem__Not sure if this will work though
image.data += np.random.rand(image.data[:].shape) or maybe it will because the @feature.setter or descriptor __set__ will call with slice(None), i.e. [:], like I showed above:
@colors.setter
def colors(self, colors: str | np.ndarray | ColorFeature):
"""set colors"""
self._colors[:] = colorsI can checkout these details later, I think for now just the ability to slice and set features after creating graphics is really what we need for all our use cases and I think that's what most users will do.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|
