Skip to content

Fix scatter3d so it handles updates to positions - #484

Closed
nvaytet wants to merge 15 commits into
mainfrom
update-positions-scatter3d
Closed

Fix scatter3d so it handles updates to positions#484
nvaytet wants to merge 15 commits into
mainfrom
update-positions-scatter3d

Conversation

@nvaytet

@nvaytetnvaytet commented Sep 5, 2025

Copy link
Copy Markdown
Member

Like the 2d scatter plots, the 3d scatter plot will now handle updates to the data that have a different number of points.

If this is the case, we have to remove the point cloud and add a new one to the scene.

@nvaytet
nvaytet marked this pull request as ready for review September 5, 2025 14:50

self._add_point_cloud_to_scene()

def _make_point_cloud(self) -> None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How long does this take approximately? Is it as fast as updating a 2d plot?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It depends on the number of points. I did some basic timings: it's about 0.01s for 100_000 points, and ~0.04s for 500_000 points.
But this is only run if the updated values have a different shape than the existing ones, which I am guessing is not super common.

A more relevant question is probably how long does it take to update the positions every time?
We are now running

self.geometry.attributes["position"].array=np.array(
[
self._data.coords[self._x].values.astype('float32'),
self._data.coords[self._y].values.astype('float32'),
self._data.coords[self._z].values.astype('float32'),
]
).T

on every update, which is potentially quite a large allocation?
Before, we only have one array of floats for the colors, now we have 4 (colors + 3 positions).

We could only update if the coords have changed, but we would have to check something like

ifany(notsc.identical(old_coords[dim], self._data.coords[dim]) fordimin"xyz"):

I need to check the timings of such a check, maybe it's fast enough?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, check it before you optimise too much! 0.04s seems fine. I don't think anyone expects 60fps.

Just a guess, but maybe you can make the big allocation slightly cheaper by using np.stack or any of its variants instead of np.array([..]).T.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The check can actually be pretty fast (10x or more) compared to setting the position, so I added it in.

Comment threadsrc/plopp/backends/pythreejs/scatter3d.py Outdated
self._update_colors()

if need_new_point_cloud:
self._add_point_cloud_to_scene()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you have to remove the old cloud?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is done above on L111, but maybe it makes more sense to do it here... and remove of the if self.points is not None

@nvaytet
nvaytet marked this pull request as draft September 8, 2025 19:21
@nvaytet
nvaytet marked this pull request as ready for review September 9, 2025 07:40
@nvaytet

nvaytet commented Sep 9, 2025

Copy link
Copy Markdown
MemberAuthor

@jl-wynen this is now ready for another look.
I dug deeper into how things were updated and realized we were setting the colors twice instead of once on every update. So this should also improved performance on large plots (such as the DREAM instrument view).

The logic is probably not super easy to follow from the diff, so we can do a run-through if you'd like that.
I'm also not super happy with member functions that both set attributes on the class and return something (e.g. here), so maybe we can improve that further?

@nvaytet
nvaytet marked this pull request as draft September 10, 2025 07:18
@nvaytet

Copy link
Copy Markdown
MemberAuthor

Hold on, there are some issues with the 3d clipping...

@nvaytet

Copy link
Copy Markdown
MemberAuthor

I need to investigate further. The performance impact of changes here is very noticeable on e.g. the DREAM instrument view...

@nvaytet

Copy link
Copy Markdown
MemberAuthor

Closing in favour of #559

@nvaytetnvaytet closed this Apr 27, 2026
@github-project-automationgithub-project-automationBot moved this from In progress to Done in Development BoardApr 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants

@nvaytet@jl-wynen