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

Feature/colors for fish dataset #77

Merged
merged 5 commits into from
Oct 21, 2021
Merged

Conversation

toloudis
Copy link
Contributor

Problem

The FISH dataset was showing a poorly selected set of channels for its 3d display of images.

Solution

We decided what channels and colors to use for the fish dataset. I set the colors in our codebase in a section that lets us make choices based on the name of the dataset selected.

Unfortunately I also had to update the eslint / parser versions and then patch up a whole bunch of linter warnings and errors.

Type of change

  • New feature (non-breaking change which adds functionality)

Change summary:

I will highlight this with comments in the PR at key points in the code.

Steps to Verify:

  1. Load FISH dataset and verify that only channels 0,1,3,4 are visible and with the properly selected colors

defaultSurfacesOn={[]}
initialChannelSettings={props.initialChannelSettings}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes in this file propagate the dataset's 3d viewer settings down into the actual viewer.

"4": { color: [255, 255, 255] },
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes in this file show the new dataset-dependent settings. Really, nearly ALL of this file should be in json in the dataset itself and none of it should be hardcoded here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that we can actually pick custom channel colors quite easily here. initialChannelSettings also gives some control over the initial thresholding settings but I didn't expose it here in these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The changes in this file show the new dataset-dependent settings. Really, nearly ALL of this file should be in json in the dataset itself and none of it should be hardcoded here.

Do we have a Jira issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt it. I'll make one if there isn't

@@ -26,6 +26,8 @@ export interface VolumeViewerProps {
cellDownloadHref: string;
channelNameMapping?: { label: string; test: RegExp }[] | "";
groupToChannelNameMap?: { [key: string]: string[] } | "";
channelsEnabled?: number[];
initialChannelSettings?: { [key: string]: { color: [number, number, number] } };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these two fields had to be added to match up with props passed to the 3d viewer.

"hoverClosestCartesian" as "hoverClosestCartesian",
"hoverCompareCartesian" as "hoverCompareCartesian",
"toggleSpikelines" as "toggleSpikelines",
"sendDataToCloud" as const,
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen this before, why is this needed? (and why not "string" or what it was before)?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh is it the same as const sendDataToCloud = "sendDataToCloud" and then saying type sendDataToCloud = typeof sendDataToCloud?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was one of the linting fixes. there's a eslint rule that says to prefer const. I tried removing the as const but that was definitely still a typeCheck fail.

Copy link
Contributor

@schoinh schoinh left a comment

Choose a reason for hiding this comment

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

Thanks for making the linter work / fixing errors!

@toloudis toloudis merged commit 3925ea9 into main Oct 21, 2021
@toloudis toloudis deleted the feature/colors-for-fish-dataset branch October 21, 2021 17:57
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.

3 participants