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

need to improve how we handle buffers #147

Closed
kushalkolar opened this issue Mar 6, 2023 · 2 comments · Fixed by #511
Closed

need to improve how we handle buffers #147

kushalkolar opened this issue Mar 6, 2023 · 2 comments · Fixed by #511
Labels
enhancement New feature or request

Comments

@kushalkolar
Copy link
Member

I just noticed that before #143 when we initialized any GraphicFeature, the data were being copied to a new array because of astype(np.float32) which has copy=True by default: https://github.com/kushalkolar/fastplotlib/pull/143/files#diff-e9a4f655a73cc26eb39868b9b4f44940cccfa247f6c433559cec77954383a198L46

We need to think about how to properly deal with updating the data in these buffers when the arrays are read-only, such as numpy memmaps:

>>> img_graphic.data._buffer.data

memmap([[ 14.957001 ,   8.239777 ,  24.973755 , ...,   2.7485046,
           5.922577 ,   3.677765 ],
        [ 91.104004 ,  39.25052  ,   6.083435 , ...,  38.935028 ,
          57.211304 ,  23.596008 ],
        [121.86125  ,  74.614716 ,  20.688904 , ...,  25.005035 ,
          73.836426 ,  12.158752 ],
        ...,
        [ 35.85089  ,  22.899536 ,  21.668213 , ...,  93.56639  ,
          38.996155 ,  32.658997 ],
        [ 15.931152 ,  12.423279 ,  10.606384 , ...,  64.8004   ,
          21.833801 ,   8.394348 ],
        [ 54.492004 ,   6.143982 ,  18.702332 , ...,  22.917175 ,
          15.810699 ,  30.137512 ]], dtype=float32)

Trying to assign, results in the following:

>> img_graphic.data = 0
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Input In [44], in <cell line: 1>()
----> 1 iw_cnmf.plot.graphics[0].data[0] = 0

File ~/Insync/gdrive/repos/fastplotlib/fastplotlib/graphics/features/_data.py:114, in ImageDataFeature.__setitem__(self, key, value)
    110 def __setitem__(self, key, value):
    111     # make sure float32
    112     value = to_gpu_supported_dtype(value)
--> 114     self._buffer.data[key] = value
    115     self._update_range(key)
    117     # avoid creating dicts constantly if there are no events to handle

ValueError: assignment destination is read-only

But we are only showing a part of the entire large memmap in those cases so we could just set the texture data entirely if it's a lazy-loading type, maybe a nice way to do it would be to have something like:

import fastplotlib
from numpy import memmap
from mesmerize_core.arrays import LazyArray

fastplotlib.config.lazy_array_types.append(memmap, LazyArray)
``

And then in the various `GraphicFeatureIndexable._set()` we can set `_buffer.data` entirely if they are one of those `lazy_array_types`.
@kushalkolar kushalkolar added the bug Something isn't working label Mar 9, 2023
@kushalkolar
Copy link
Member Author

kushalkolar commented Mar 9, 2023

  1. Refactor indexable graphics, don't have __call__ to get the data under the buffer, make it explicit so it has to be feature.buffer.data to get the data.
  2. A kwarg for isolating buffers with this type of usage:
    • common_buffer=None - all buffers are isolated from input arrays
    • common_buffer="data"
    • common_buffer=["data", "colors"] - when using non-isolated buffer for colors, an array must be provided for the colors.
  3. If a common buffer is specified for any feature, then the dtype of the input array must be GPU compatible else raise

@kushalkolar
Copy link
Member Author

Consider making features properties. This way we can define the set behavior better without the awkward way it's done using setattr in Graphic

@kushalkolar kushalkolar added enhancement New feature or request and removed bug Something isn't working labels Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant