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

Color by attribute #148

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Color by attribute #148

wants to merge 30 commits into from

Conversation

TeunHuijben
Copy link
Collaborator

@TeunHuijben TeunHuijben commented Nov 6, 2024

I closed #147, and started a new one on this topic:

This PR will implement the option to color the cells based on some attribute. The attribute could be any column added to the tracks.csv, or calculated features from the coordinates (think of velocity, direction, etc.)

Implemented:

  • dropdown window with attribute options
  • embed extra columns from tracks.csv into Zarr Store
  • update dropdown window options based on fields in .zattrs
  • load extra Zarr fields when attributes are chosen in the dropdown
  • use a proper colormap
  • use the CZI-SDS inputDropDown (not mui/material)
  • add legend/colormap indicator
  • add colormaps (categorical/continuous) and move them to a general location (together with trackMaterial colormaps)
  • config option to choose colormaps (for categorical/continuous attributes)
  • add toggle button to enable/disable coloring cell by attribute
  • let the conversion script detect categorical/continuous features
  • make test for conversion script with attributes
  • give conversion script the option to add specific columns (instead of all columns)
  • dropdown/toggle only visible when attributes are provided
  • colorBy state saved in viewerState, to maintain when copying URL
  • config option to deactivate default attributes (x-pos, y-pos, etc.)
  • config option to deactivate this features all-together (even if attributes are provided)
  • reset dropdown when new dataset loaded

Bugs:

  • inputText for dataURL no longer turns red when URL is false (the team behind SDS is working on this)

Example datasets:

Example of coloring based on z-coordinate:

Screenshot from 2024-11-21 16-41-35

Copy link

vercel bot commented Nov 6, 2024

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

Name Status Preview Updated (UTC)
intracktive ✅ Ready (Inspect) Visit Preview Dec 7, 2024 1:33am

@TeunHuijben TeunHuijben added the enhancement New feature or request label Nov 15, 2024
@TeunHuijben
Copy link
Collaborator Author

TeunHuijben commented Dec 7, 2024

Hi @andy-sweet and @aganders3,

This PR is a bit bigger and contains the feature to color cells based on provided attributes. In short, 1) extra columns in the tracks.csv file are saved in the Zarr Store, 2) the UI has a toggle and a dropdown menu where the user can select the feature used for coloring the cells. The Zarr Store encodes the names of the features and the type (categorical/continuous). The user can set the colormaps in the CONFIG, and there are separate colormaps for categorical/continuous features.

Considerations for review:

  • is this the best way to save/chunk the attributes into the Zarr Store?
  • for the larger datasets (zebrafish), I feel that coloring the cells according to a attribute that is loaded from the Zarr Store, make the viewer slower. Would you see any opportunity for speed-up? (the fastest way to would be to add the features to the points Zarr array ([x,y,z,attribute1,attribute2,...,x,y,z,attribute1,attribute2], but this would create a very large points array in the case of many attributes, especially since most of them are usually not used)
  • the "quadrants" coloring option is to show categorical features, not because it is useful

Thanks!

@TeunHuijben TeunHuijben marked this pull request as ready for review December 9, 2024 01:58
@aganders3
Copy link
Collaborator

It looks like you're creating another ragged array in the top-level of the Zarr bundle as attributes that is roughly the same shape as points (well, num_attributes * 1/3 the size). Is that right? With the same chunking as points this should only be one extra request when the timepoint changes - I don't think that should be a huge deal but it is some overhead.

I agree packing it all into points is probably not optimal if there will be a lot of unused attributes. You may have the same issue in attributes if you have a lot of attributes saved. What about making attributes another group, and inside you could have separate arrays for each attribute?

@TeunHuijben
Copy link
Collaborator Author

Hi @aganders3, thanks for having a look! Yes, that is correct, attributes is a dense ragged array. The chunking is different from points though, where points is chunked per time, attributes is also chunked per attribute. In this way, only the requested attribute at the current timepoint is fetched (I hope I implemented this correctly).

What do you mean with "making attributes another group"? Do you mean to make a second zarr store with only the attributes? Or can we make a second group within the tracks_bundle.zarr store? I might be unfamiliar with the definitions or store/group/array. Because right now, attributes is an extra (the fifth) array within the same store.

@aganders3
Copy link
Collaborator

The chunking is different from points though, where points is chunked per time, attributes is also chunked per attribute.

Ahh, I see. I missed this distinction and thought the attributes were still interleaved. Never mind my suggestion then, I think this achieves the same thing!

Copy link
Collaborator

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

I'm still making my way through this, but left lots of minor comments and one or two more important ones. Will try to take another look tomorrow.

Qualitatively, I noticed a minor performance hit, but not enough of one to block this feature which seems really useful!

@@ -11,16 +11,26 @@ const config = {
// When opening the viewer, or refreshing the page, the viewer will revert to the following default dataset
data:{
// Default dataset URL (must be publically accessible)
default_dataset: "https://public.czbiohub.org/royerlab/zoo/Zebrafish/tracks_zebrafish_bundle.zarr/"
// default_dataset: "https://public.czbiohub.org/royerlab/zoo/Zebrafish/tracks_zebrafish_bundle.zarr/"
default_dataset: "https://public.czbiohub.org/royerlab/zoo/misc/tracks_drosophila_attributes_norm_bundle.zarr/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be the following?

Suggested change
default_dataset: "https://public.czbiohub.org/royerlab/zoo/misc/tracks_drosophila_attributes_norm_bundle.zarr/"
default_dataset: "https://public.czbiohub.org/royerlab/zoo/misc/tracks_zebrafish_displ_norm_bundle.zarr/"

@@ -117,6 +117,13 @@ intracktive convert --csv_file path/to/tracks.csv --add_radius

Or use `intracktive convert --help` for the documentation on the inputs and outputs

**ToDo: explain how to add attributes**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this TODO still need to be done?

@@ -223,17 +225,26 @@ export default function App() {
const getPoints = async (time: number) => {
console.debug("fetch points at time %d", time);
const data = await trackManager.fetchPointsAtTime(time);
// console.log('data shape:', data.length, 'attributes shape:', attributes.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe combine this with the debug log below? Or remove it.

const [inputDropdownValue, setInputDropdownValue] = useState<string | undefined>(options[0]?.name);
const [value, setValue] = useState<AutocompleteValue<Option, false, false, false> | null>(options[0] || null);

// useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be removed? If not add a comment to the code to explain why it might be useful in the future.

<DropdownMenu
open={open}
anchorEl={anchorEl}
// onClose={noop}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still needed?

Comment on lines +430 to +432
// const numPoints = positions.count / numberOfValuesPerPoint;
// console.log('numPoints in getAtt:',numPoints, positions.count, numberOfValuesPerPoint)
// console.log('positions:',positions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// const numPoints = positions.count / numberOfValuesPerPoint;
// console.log('numPoints in getAtt:',numPoints, positions.count, numberOfValuesPerPoint)
// console.log('positions:',positions)

const quadrant = x + y * 2 + z * 4; //
attributeVector.push(quadrant); // color based on XY coordinates (4 groups)
} else {
attributeVector.push(1); // default to constant color if event type not recognized
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe an error should be logged in this case?

@@ -153,6 +157,28 @@ export class TrackManager {
return array;
}

async fetchAttributessAtTime(timeIndex: number, attributeIndex: number): Promise<Float32Array> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
async fetchAttributessAtTime(timeIndex: number, attributeIndex: number): Promise<Float32Array> {
async fetchAttributesAtTime(timeIndex: number, attributeIndex: number): Promise<Float32Array> {


export const numberOfDefaultColorByOptions = DEFAULT_DROPDOWN_OPTIONS.length;
// Initialize the mutable dropdown options with the default options
export const dropDownOptions: Option[] = [...DEFAULT_DROPDOWN_OPTIONS];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't made a drop-down in SDS, but the code here feels problematic on a first look because this is a mutable array of Options that is effectively used as React state.

For react state, the typical pattern is to just create a new array each time one is needed rather than mutate a single instance.


return (
<div>
<InputDropdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the SDS docs, I'd expect to see DropDown here instead of InputDropdown and DropdownMenu.

But I've never actually used any of those, so probably ignore?

Copy link
Collaborator

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

I think the zarr storage format and fetching (when needed) is as reasonable as prior choices we've made, so I don't have any feedback related to that or the Python conversion code/tools.

My main comment is about the how you handle the state related to the attribute drop-down box value changing. I previously left some related comments, but I made those a bit more tangible by trying some changes that I think simplify things and also fix some bugs: https://github.com/royerlab/inTRACKtive/compare/color-by-attribute...andy-sweet/refactor-attr-options?expand=1#diff-8e6c274c4241821e809d16fcd06bc109ba41bb7a0583f9ad766ed92bb8230671R8

I don't think you should blindly merge those changes, but I think the approach there is easier to follow and importantly does not make the library code depend on definitions from React components.

{ name: "y-position", label: 2, type: "continuous", action: "calculate", numCategorical: undefined },
{ name: "z-position", label: 3, type: "continuous", action: "calculate", numCategorical: undefined },
{ name: "sign(x-pos)", label: 4, type: "categorical", action: "calculate", numCategorical: 2 },
{ name: "quadrants", label: 4, type: "categorical", action: "calculate", numCategorical: 8 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming these two options should not have the same label.

Suggested change
{ name: "quadrants", label: 4, type: "categorical", action: "calculate", numCategorical: 8 },
{ name: "quadrants", label: 5, type: "categorical", action: "calculate", numCategorical: 8 },

@@ -24,13 +26,17 @@ import { Track } from "@/lib/three/Track";
import { PointSelector, PointSelectionMode } from "@/lib/PointSelector";
import { ViewerState } from "./ViewerState";
import { numberOfValuesPerPoint } from "./TrackManager";
import { Option, dropDownOptions } from "@/components/leftSidebar/DynamicDropdown";
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a rule, we tried to avoid make the code in lib depend on definitions in components, so we can define the core logic of the library and rendering independently of React.

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.

3 participants