Scatterbrain rendering in vis, including shader generation#223
Scatterbrain rendering in vis, including shader generation#223
Conversation
… demo. initial thoughts
…tor decide to continue traversal (or not) in addition to whatever effectful thing it would normally do
…out and start smaller...
… about how to generate these shaders in a less hard to modify way
| export function visitBFSMaybe<Tree>( | ||
| tree: Tree, | ||
| children: (t: Tree) => ReadonlyArray<Tree>, | ||
| visitor: (tree: Tree) => boolean, |
There was a problem hiding this comment.
a slight variation on the previous visitBFS pattern - this one packs the choice to proceed into the visitor - the idea is that the visitor performs some side-effect, and returns true if recursion should proceed.
packages/scatterbrain/src/dataset.ts
Outdated
| import type { ScatterbrainDataset, SlideviewScatterbrainDataset, TreeNode, volumeBound } from "./types"; | ||
| import reduce from "lodash/reduce"; | ||
|
|
||
| type Dataset = ScatterbrainDataset | SlideviewScatterbrainDataset |
There was a problem hiding this comment.
I tried to pull in and hopefully better organize the code for loading and traversing these datasets
packages/scatterbrain/src/dataset.ts
Outdated
|
|
||
| export function loadDataset(raw: any): Dataset | undefined { | ||
| // index point attrs by name - its an array | ||
| // TODO zod validation first! |
There was a problem hiding this comment.
yup - I need to do this
| type Content = Record<string, VBO> | ||
|
|
||
|
|
||
| export function buildScatterbrainCacheClient(regl: REGL.Regl, cache: SharedPriorityCache, onDataArrived: () => void) { |
There was a problem hiding this comment.
here is our cache client implementation - it handles the fetching of the various types of data (genes or metadata) and uploading into GPU buffers
| } | ||
| } | ||
|
|
||
| function buildHelperThingy(regl: REGL.Regl, state: ShaderSettings) { |
There was a problem hiding this comment.
this short function has a silly name - its probably better to just move it into the one place its called
There was a problem hiding this comment.
agree, I can't really come up with a better name than qtCellHelper :D
| } | ||
| } | ||
|
|
||
| /* TODO features: |
There was a problem hiding this comment.
remove this comment
| import { Box2D, type vec4, type box2D, type Interval, type vec2 } from "@alleninstitute/vis-geometry"; | ||
| import { type Cacheable, type CachedVertexBuffer } from "@alleninstitute/vis-core"; | ||
|
|
||
| // the set of columns and what to do with them can vary |
There was a problem hiding this comment.
consider moving this commentary to a readme, or at least making it less rambly
There was a problem hiding this comment.
I'm okay if this is here for now. I've been exploring producing documentation automatically from JSDoc comments, or having us enhance our README docs and get those deployed on the vis website (or maybe both options!).
Maybe turn into a JS Doc comment attached to the VBO class though?
| // get without a great deal of extra performance cost | ||
|
|
||
|
|
||
| // scatterbrain does scatterplot rendering |
There was a problem hiding this comment.
this file is worth looking at closely. Its shorter and more centralized than the original ABC-atlas implementation, and I really hope its more readable. I think its got some nice guide-rails for how to think about making these, but because we still compose strings, it can be easy to make spelling or GLSL syntax mistakes.
packages/scatterbrain/src/shader.ts
Outdated
|
|
||
| export function generate(config: Config): ScatterbrainShaderUtils { | ||
| const { mode, quantitativeColumns, categoricalColumns, categoricalTable, tableSize, gradientTable, positionColumn, colorByColumn } = config; | ||
| console.log('tableSize: ', tableSize) |
There was a problem hiding this comment.
remove console logs
| const getDataPosition = /*glsl*/`return vec3(${positionColumn}+offset,0.0);` | ||
| const getClipPosition = /*glsl*/`return applyCamera(getDataPosition());` | ||
| const getPointSize = /*glsl*/`return mix(2.0,6.0,isHovered());` // todo! | ||
| // todo - use config options! |
There was a problem hiding this comment.
we are missing some features here - configurable point size,
and easily-configurable depth behavior
packages/scatterbrain/src/shader.ts
Outdated
| // produce an object that can be used to set up some internal config of the shader that would | ||
| // do the visualization | ||
| const { dataset, categoricalFilters, quantitativeFilters, colorBy, mode } = settings; | ||
| console.log('cat filters...', categoricalFilters) |
packages/scatterbrain/src/types.ts
Outdated
| @@ -0,0 +1,88 @@ | |||
| // lets get a renderer up and rolling | |||
| // then add features from there... | |||
There was a problem hiding this comment.
remove commentary
packages/scatterbrain/demo.ts
Outdated
| import { ShaderSettings } from "./src/shader"; | ||
| import { vec4 } from "@alleninstitute/vis-geometry"; | ||
|
|
||
| const twoGB = 1024 * 1024 * 2000; |
There was a problem hiding this comment.
this is a demo I made in the package itself, because it was a lot easier than doing the same in the site/examples area. Now that we're here, I would be happy to delete it.
There was a problem hiding this comment.
I'm not seeing it work right now, so yeah let's either fix it or delete it!
packages/scatterbrain/index.html
Outdated
| height: 100%; | ||
| } | ||
| </style> | ||
| <script defer src="demo.ts" type="module"></script> |
There was a problem hiding this comment.
this is the entrypoint for the "non-example" demo
| multiple gene coloring / filtering | ||
| slide-view / regular view | ||
|
|
||
| to this end, we are gonna stick to the Renderer<> interface as given in packages/core/src/abstract/types.ts. We've seen that work with both the somewhat Ill-fated renderServer, as well as the new shared-cache |
There was a problem hiding this comment.
well - I didnt follow the advice on this line, but I think thats ok. update this commentary though!
| } | ||
| // calling a texture as a function is REGL shorthand for total re-init of this texture, capable of resizing if needed | ||
| // warning - this is not likely to be fast | ||
| texture({ data, width: columns, height: rows }); |
There was a problem hiding this comment.
is more performance here potentially needed?
There was a problem hiding this comment.
I suspect not - changing filtering is a user action, so a few milliseconds of upload time is reasonable
packages/scatterbrain/src/shader.ts
Outdated
| function rangeFilterExpression(qColumns: readonly string[]) { | ||
| return qColumns.map(attrib =>/*glsl*/`within(${attrib},${rangeFor(attrib)})`).join(' * ') | ||
| } | ||
| function categoricalFilterExpression(cColumns: readonly string[], tableSize: vec2, tableName: string) { |
There was a problem hiding this comment.
I would be more verbose with the naming of qColumns and cColumns
packages/scatterbrain/src/shader.ts
Outdated
| const categories = keys(categoricalFilters).toSorted() | ||
| const numCategories = categories.length; | ||
| const longest = reduce(keys(categoricalFilters), (highest, cur) => Math.max(highest, categoricalFilters[cur]), 0) | ||
| const qAttrs = reduce(quantitativeFilters.toSorted(), (acc, cur, i) => ({ ...acc, [cur]: `MEASURE_${i.toFixed(0)}` }), colorBy.kind === 'metadata' ? {} : { [colorBy.column]: 'COLOR_BY_MEASURE' } as Record<string, string>); |
There was a problem hiding this comment.
One alternative is:
const qAttrs: Record<string, string> = {
...(colorBy.kind === 'metadata' ? {} : { [colorBy.column]: 'COLOR_BY_MEASURE' }),
...Object.fromEntries(quantitativeFilters.toSorted().map((x, i) => [x,`MEASURE_${i.toFixed(0)}`])),
};I find it easier to read but I'm not as used to reduce
There was a problem hiding this comment.
hmmm, fair enough - I'll see if I can improve on the legibility of this area a bit
| }, | ||
| isValue: (v): v is Content => { | ||
| // TODO!! | ||
| // a problem here - unless we do some pre-cooking of shaders... the set of attrs we pass to the shader |
There was a problem hiding this comment.
I don't exactly follow the problem but this might be a good reason to follow a pattern similar to ShaderBuilder as I mentioned above. Neuroglancer is designed to generate the simplest shader necessary based on the user settings and we don't want to slow down the rendering in any way when wanting to also optionally support a more expensive setting.
lanesawyer
left a comment
There was a problem hiding this comment.
Haven't dug into the meat of it yet but have a few comments so far
| while (frontier.length > 0) { | ||
| const cur = frontier.shift(); | ||
| if (cur === undefined) { | ||
| // TODO: Consider logging a warning or error here, as this should never happen, |
There was a problem hiding this comment.
Hm. Maybe we make core a peerDependency of geometry and use the peerDependenciesMeta` to mark it as optional, and then write a bit of defensive code by checking if the logger exists before using it?
I hadn't know about the meta field until I tried to look for stuff to solve this issue just now, it's nice! Wish you could list why or what features are not going to work without it though...
https://docs.npmjs.com/cli/v10/configuring-npm/package-json?v=true#peerdependenciesmeta
There was a problem hiding this comment.
ok - so I stared at this for a bit, and I'm convinced this todo is pretty clearly impossible to hit, so I dont think its worth making any changes now
There was a problem hiding this comment.
I'll update the comment and remove the todo
packages/geometry/package.json
Outdated
| "type": "git", | ||
| "url": "https://github.com/AllenInstitute/vis.git" | ||
| }, | ||
| "devDependencies": { |
There was a problem hiding this comment.
I don't think these changes are needed, it should pull from the root package.json for all of these. I wouldn't expect any changes to this file for the addition of the new BFS method.
There was a problem hiding this comment.
yes, these are probably leftovers from an experiment
packages/scatterbrain/package.json
Outdated
| "type": "git", | ||
| "url": "https://github.com/AllenInstitute/vis.git" | ||
| }, | ||
| "devDependencies": { |
There was a problem hiding this comment.
These will all be provided by the workspace package.json, can remove the whole devDependencies section
packages/scatterbrain/package.json
Outdated
| "@types/lodash": "4.17.21", | ||
| "parcel": "2.15.4", | ||
| "typescript": "5.9.3", | ||
| "vite": "7.3.0", |
There was a problem hiding this comment.
Although you'd actually need vite in here if you keep the demo in packages/scatterbrain around
packages/scatterbrain/demo.ts
Outdated
| import { ShaderSettings } from "./src/shader"; | ||
| import { vec4 } from "@alleninstitute/vis-geometry"; | ||
|
|
||
| const twoGB = 1024 * 1024 * 2000; |
There was a problem hiding this comment.
I'm not seeing it work right now, so yeah let's either fix it or delete it!
| // make up random colors for the coloring, and add random filtering | ||
|
|
||
| setCategoricalLookupTableValues(categories, lookup) | ||
| const render = buildScatterbrainRenderFn(regl as any, {...settings,dataset}); |
There was a problem hiding this comment.
yeahhhh the any is very weird. couldn't find any immediate fixes though. but it makes me nervous that there will be type issues when pulled into bkp-client so it would be nice to get this figured out
There was a problem hiding this comment.
this is the same issue that is 'resolved' via the // @ ts-expect-error issues elsewhere. I'll change this instance to match those
There was a problem hiding this comment.
We should probably pull these into the site. Not all packages have a README.md right now though. No need to do it in this PR, I'm just commenting to cement the idea better in my brain for later!
| { | ||
| "name": "@alleninstitute/vis-scatterbrain", | ||
| "version": "0.0.1", | ||
| "contributors": [ |
There was a problem hiding this comment.
Now here's a tough one. should contributors just be you, since you committed all the code in this PR? or should we figure out who helped write whatever you copy/pasted from the bkp-client?
… from geometry, fix commentary about a bit of dead code
lanesawyer
left a comment
There was a problem hiding this comment.
Looking good, demo still working just fine, couple a comments here and there for ya'
| return proms; | ||
| }, | ||
| isValue: (v): v is Content => { | ||
| // TODO |
| return client; | ||
| } | ||
|
|
||
| type State = ShaderSettings & { |
There was a problem hiding this comment.
I'm not seeing this type used anywhere
| texture: REGL.Texture2D | ||
| ) { | ||
| const { category, row, color, filteredIn } = update; | ||
| const col = categories.toSorted().indexOf(category) |
There was a problem hiding this comment.
I'm trying to figure out if the toSorted is for performance reasons, since maybe if we sort then the index gets hit soon, or if toSorted is to match the ordering elsewhere (like setCategoricalLookupTableValues, maybe, or in shader.ts? Or is there another reason I'm not thinking of?
If it's the former, I'm not totally convinced it would be faster.
If it's the latter, that feels a little dangerous to rely on the sort order of this being the same as it's sort order elsewhere and it would be nice to get that category access coming from a common place in the code so we don't introduce bugs in the future in case we need to access categories in a sorted manner again or tweak one of the places it's being sorted.
There was a problem hiding this comment.
so what is happening here - first a little background:
the shader uses a lookup table to power categorical filtering and coloring - each column has to have a hard-coded index in the shader. that column's index in the table, and thus in the generated shader, corresponds to a vertex attribute that is used as the key to lookup color or filtering status of a point for that property.
what that means is that the set of columns used to create the texture must match the columns used to generate the shader - to my mind, this is made a bit more simple and stable by sorting the columns by name.
regarding performance - we tend to see just a few columns in use at a time - sorting a list of a few dozen strings repeatedly shouldn't be an issue. having the sorting done within the renderer / shader generator modules lets the external API simply pass a record of categories without having to bother with the sorting.
to your point - we could re-organize this so that the use of the lookup table is opaque... but then we'd have to diff the columns and handle the memory for the lookup table in a more conservative manner to avoid leaks... might be a good idea though, I'm on the fence
There was a problem hiding this comment.
Makes sense, yeah we'd have to refactor it quite heavily to centralize the sorting/lookup logic. I'm confident in our ability to not break it as-is though, so I'm fine leaving it this way!
| const { category, row, color, filteredIn } = update; | ||
| const col = categories.toSorted().indexOf(category) | ||
| if (texture.width <= col || texture.height <= row || row < 0 || col < 0) { | ||
| // todo - it might be better to let regl throw the same error... think about it |
There was a problem hiding this comment.
TODO here, are you saying regl will throw the error anyway if we get it wrong?
| import { Box2D, type vec4, type box2D, type Interval, type vec2 } from "@alleninstitute/vis-geometry"; | ||
| import { type Cacheable, type CachedVertexBuffer } from "@alleninstitute/vis-core"; | ||
|
|
||
| // the set of columns and what to do with them can vary |
There was a problem hiding this comment.
I'm okay if this is here for now. I've been exploring producing documentation automatically from JSDoc comments, or having us enhance our README docs and get those deployed on the vis website (or maybe both options!).
Maybe turn into a JS Doc comment attached to the VBO class though?
lanesawyer
left a comment
There was a problem hiding this comment.
Demo still works and new code changes look good. Yay for zod! That's slowly infecting all our codebases, but it's great 😁
I called out one spot that will fix the CI jobs, once those are passing, let's get this merged!
| tableOfContents: false | ||
| --- | ||
|
|
||
| import { ScatterBrainDemo } from '../../../examples/Scatterbrain/demo.tsx'; |
There was a problem hiding this comment.
All the CI jobs are failing, I think you just need to not capitalize Scatterbrain here
| import type { vec2, vec4 } from '@alleninstitute/vis-geometry'; | ||
| import { SharedCacheContext, SharedCacheProvider } from '../common/react/priority-cache-provider'; | ||
| import { useContext, useEffect, useRef, useState } from 'react'; | ||
| import { buildScatterbrainRenderFn, buildScatterbrainCacheClient, loadScatterbrainDataset, setCategoricalLookupTableValues, type Dataset, type ShaderSettings } from '@alleninstitute/vis-scatterbrain'; |
There was a problem hiding this comment.
buildScatterbrainCacheClient is unused
| @@ -16,8 +16,7 @@ export function visitBFS<Tree>( | |||
| while (frontier.length > 0) { | |||
| const cur = frontier.shift(); | |||
There was a problem hiding this comment.
I would use a ! assertion here but I understand if you prefer to avoid it
There was a problem hiding this comment.
I like it - I tend to not use those, having stepped on that rake a few times, but its clearly reasonable here
Co-authored-by: Lane Sawyer <lane.sawyer@alleninstitute.org>
…itute/vis into noah/scatterbrain-port
lanesawyer
left a comment
There was a problem hiding this comment.
I'm happy with it, let's ship!
A Not quite feature complete implementation of basic scatterbrain scatterplot rendering
What
Replace this with 1-2 line Description of what feature, bugfix, chore does the PR contain the code for.
How
Replace this txt describing what kind of technical overlaying code changes were introduced here.
Screenshots
This section is optional if there are no visible changes
PR Checklist
main?