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

[WIP] Different radius per point #103

Merged
merged 34 commits into from
Sep 16, 2024
Merged

[WIP] Different radius per point #103

merged 34 commits into from
Sep 16, 2024

Conversation

TeunHuijben
Copy link
Collaborator

For embryos with only a few cells (10-100), showing only a scatter plot of points is too sparse to visually represent the embryo. One idea would be the give each point the "size" of the actual cell that it represents (could be all the cells the same size, or each cell a different size, dependent on the local density of cells).

In this PR, I changed TrackManager and PointCanvas to accommodate for each point having a different size. The size (radius) of each point is saved in the points_array zarr as fourth column (previously it had only 3 columns: x,y,z).

Additionally, I have created a new conversion script convert_tracks_csv_to_sparse_zarr_with_radius that works csv files that contain the 'radius' column, and incorporates this information in the zarr structure.

This PR is represents more a 'continuous' track record of my changes. It is not a final PR that should be merged with main, at this stage. Any feedback is welcome.

Copy link

vercel bot commented Aug 20, 2024

@TeunHuijben is attempting to deploy a commit to the Chan Zuckerberg Biohub SF Team on Vercel.

To accomplish this, @TeunHuijben needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

Copy link

vercel bot commented Aug 21, 2024

An owner of the Chan Zuckerberg Biohub SF Team on Vercel accepted @TeunHuijben's request to join.

@TeunHuijben's commit is now being deployed.

Copy link

vercel bot commented Aug 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
points-web-viewer ✅ Ready (Inspect) Visit Preview Sep 16, 2024 10:39pm

@aganders3
Copy link
Collaborator

This looks neat! I requested access to the preview on Vercel.

🚀

Comment on lines 211 to 221
updateSelectedPointIndices() {
const idOffset = this.curTime * this.maxPointsPerTimepoint;
this.selectedPointIndices = [];
for (const track of this.tracks.values()) {
if (this.curTime < track.threeTrack.startTime || this.curTime > track.threeTrack.endTime) continue;
const timeIndex = this.curTime - track.threeTrack.startTime;
const pointId = track.threeTrack.pointIds[timeIndex];
this.selectedPointIndices.push(pointId - idOffset);
}
this.highlightPoints(this.selectedPointIndices);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could get pretty slow with a lot tracks selected. My idea in #96 (comment) was to basically memoize this function. I think the cache can also be computed/updated when adding tracks instead of when the timepoint changes.

The tradeoff is more memory, of course. I guess it should be tested but I don't think it's that much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Ahsley, that is good point. I made a bit of a mess by including elements from #96 (@andy-sweet) in this branch without fully merging with that PR. The reason was that #96 diverged from main a lot earlier than this branch, which made merging difficult.

I have now updated updateSelectedPointIndices in the next commit by using memoization. I create a map that relates a CacheKeys to the selectedPointIndices vector. The body of this function is only evaluated the first time this combination of curTime and tracks is requested. Any future request, the vector is taken from the cache. The full cache is removed as soon as the user selects more/other cells, to prevent the cache from becoming too big.

Your suggestion of precomputing the cache every time tracks are added might be cumbersome in the case of many timepoints, but we could consider this.

The dataset here is too small to experience a speed-up, but if should be faster. Was this the idea you had in mind? Let me know if anything can be improved.

…ectedPointIndices is only updated if it hasnt been computed before for this combination of curTime and selected tracks
@TeunHuijben
Copy link
Collaborator Author

This big PR contains many features, amongst others:

  • each point can have a different size (specified in the 'points' field of the zarr)
  • whether size is included in the data is specified in the zarr attributes
  • the conversion script to zarr now has a flag that can be used to specify whether to/not to include the size
  • apart from the tracks and track highlights, the selected points stay purple throughout the visualization.
  • point highlights are caches, so no re-computation is needed when frame is already rendered before
  • config file contains point size (if not provided by zarr)
  • the appearance of the points has unfortunately changed, which was a side-effect of having to change the additive blending to normal blending (to give each point a different size). Probably because transparency is no longer possible

@TeunHuijben TeunHuijben merged commit 8179e33 into main Sep 16, 2024
2 checks passed
@TeunHuijben TeunHuijben deleted the radius-from-data branch September 16, 2024 22:49
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 this pull request may close these issues.

2 participants