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

graphic features refactor #473

Merged
merged 77 commits into from
Jun 4, 2024
Merged

graphic features refactor #473

merged 77 commits into from
Jun 4, 2024

Conversation

kushalkolar
Copy link
Member

@kushalkolar kushalkolar commented Apr 3, 2024

See #511 , I accidentally merged this into main 🤦‍♂️

@kushalkolar
Copy link
Member Author

Working on this, Graphic features should have no access to collection_index. When we emit events, we can have the graphic accessible and then the collection index of the graphic can be accessed if required.

@kushalkolar
Copy link
Member Author

kushalkolar commented May 18, 2024

ok so PointsDataFeature working, and we have negative and multi-dim slicing now 🥳

@kushalkolar
Copy link
Member Author

I should double check to make sure we don't get any off by 1 errors in the slicing and with size in update_range

@kushalkolar
Copy link
Member Author

Ideas for event adding:

# graphic base class
Graphic:
  def add_event_handler(func, "event-name")

Where "event-name" is either a feature name or pygfx event name (ex: "click" etc.). And emitted feature events use a pick_info dict that holds event information just like pygfx events. Might make sense to subclass pygfx.Event here for feature events, will see.

@kushalkolar
Copy link
Member Author

@almarklein I've arrived at this parser that takes the key used to index the buffer and auto determines the offset & size to mark for upload, do you think this would be useful to have directly in pygfx?

snippet:

    def _update_range(self, key: int | slice | np.ndarray[int | bool] | list[bool | int] | tuple[slice, ...]):
        """
        Uses key from slicing to determine the offset and
        size of the buffer to mark for upload to the GPU
        """
        # number of elements in the buffer
        upper_bound = self.value.shape[0]

        if isinstance(key, tuple):
            # if multiple dims are sliced, we only need the key for
            # the first dimension corresponding to n_datapoints
            key: int | np.ndarray[int | bool] | slice = key[0]

        if isinstance(key, int):
            # simplest case
            offset = key
            size = 1

        elif isinstance(key, slice):
            # parse slice
            start, stop, step = key.indices(upper_bound)

            # account for backwards indexing
            if (start > stop) and step < 0:
                offset = stop
            else:
                offset = start

            # slice.indices will give -1 if None is passed
            # which just means 0 here since buffers do not
            # use negative indexing
            offset = max(0, offset)

            # number of elements to upload
            # this is indexing so do not add 1
            size = abs(stop - start)

        elif isinstance(key, (np.ndarray, list)):
            if isinstance(key, list):
                # convert to array
                key = np.array(key)

            if not key.ndim == 1:
                raise TypeError(key)

            if key.dtype == bool:
                # convert bool mask to integer indices
                key = np.nonzero(key)[0]

            if not np.issubdtype(key.dtype, np.integer):
                # fancy indexing doesn't make sense with non-integer types
                raise TypeError(key)

            if key.size < 1:
                # nothing to update
                return

            # convert any negative integer indices to positive indices
            key %= upper_bound

            # index of first element to upload
            offset = key.min()

            # number of elements to upload
            # add 1 because this is direct
            # passing of indices, not a start:stop
            size = np.ptp(key) + 1

        else:
            raise TypeError(key)

        self.buffer.update_range(offset=offset, size=size)

@kushalkolar
Copy link
Member Author

I need to do a thing in the tests where we just draw frames on demand to test that the right things are in buffer._gfx_pending_uploads, and that after a frame is draw this list is cleared and take a screenshot and see that it looks right.

Copy link
Member

@clewis7 clewis7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started reviewing things...

Things with lines appear to be working well - shared buffers, event handling

Images are a bit wonking - updating data, changing cmap, etc.

fastplotlib/graphics/_base.py Show resolved Hide resolved
fastplotlib/graphics/_base.py Show resolved Hide resolved
+============+=============+===============================================+
| type | str | "colors" - name of the event |
+------------+-------------+-----------------------------------------------+
| graphic | Graphic | graphic instance that the event is from |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| graphic | Graphic | graphic instance that the event is from |
| graphic | Graphic | graphic instance that emitted the event |

fastplotlib/graphics/_features/_data.py Outdated Show resolved Hide resolved
fastplotlib/graphics/_features/_image.py Show resolved Hide resolved
@kushalkolar
Copy link
Member Author

Thought from Billy: make the simplest use cases default kwargs, ex: uniform_colors for line and scatter.

@clewis7
Copy link
Member

clewis7 commented Jun 4, 2024

Adding a LinearRegionSelector to an ImageGraphic has a default fill that is solid:
image
Especially now that heatmap is part of ImageGraphic, we should pass the fill so that it is transparent, just so then you can see the data that you are selecting

@clewis7
Copy link
Member

clewis7 commented Jun 4, 2024

TODO: need to fix HLUT in ImageWidget to mirror changes in LinearRegionSelector

Comment on lines +308 to +310
if self._cmap_values is not None:
if not isinstance(self._cmap_values, np.ndarray):
raise TypeError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a previous example with lines we had setting the cmap_values from a list, with this change we would no longer be supporting

for now, I changed the example to just cast the list to an array

@clewis7 clewis7 mentioned this pull request Jun 4, 2024
@kushalkolar kushalkolar merged commit aa1e974 into main Jun 4, 2024
3 of 7 checks passed
@kushalkolar
Copy link
Member Author

kushalkolar commented Jun 4, 2024

see if this can reopen

Edit: apparently not

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

Successfully merging this pull request may close these issues.

None yet

3 participants