Color Customization V1 #2824

lanzhenw · 2023-03-21T17:04:21Z

Added color scheme set up feature:

User can set up the color setting in the UI to specify customized rules to display embedded document object's color setting.
User can apply color scheme rules in the SDK by defining session.color_scheme
User can save color scheme of the dataset to dataset.app_config

In loading, session color scheme >> dataset.app_config >> default app setting

Setting colors in the UI:
https://user-images.githubusercontent.com/17770824/234972360-44ea9c70-6a5e-4552-b7f3-5f231329c8f5.mp4

Setting the colors in the SDK:
https://user-images.githubusercontent.com/17770824/234973226-6a65fbc2-46db-428d-91f6-995b543c0069.mp4

Save to dataset:
https://user-images.githubusercontent.com/17770824/234973275-374a1c14-4977-4dbf-8814-f098f6121cca.mp4

How to test it?

import fiftyone as fo
import fiftyone.zoo as foz


dataset = foz.load_zoo_dataset("quickstart")
session = fo.launch_app(dataset)
    
color1 = fo.ColorScheme(color_pool=['blue', 'green', 'yellow', 'red'], customized_color_settings=[{'field': 'ground_truth', 'useFieldColor': True, 'fieldColor': 'pink',  'labelColors': [{'name': 'dog', 'color': 'purple'}, { 'name': 'cat', 'color': 'pink'}]}])
session.color_scheme = color1

What changes are proposed in this pull request?

How is this patch tested? If it is not, please explain why.

(Details)

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release
notes for FiftyOne users.

(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)

What areas of FiftyOne does this PR affect?

App: FiftyOne application changes
Build: Build and test infrastructure changes
Core: Core fiftyone Python library changes
Documentation: FiftyOne documentation changes
Other

codecov · 2023-03-24T16:36:28Z

Codecov Report

Patch coverage: 60.49% and project coverage change: -0.01 ⚠️

Comparison is base (71b5ea5) 62.00% compared to head (fe19f63) 62.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2824      +/-   ##
===========================================
- Coverage    62.00%   62.00%   -0.01%     
===========================================
  Files          260      264       +4     
  Lines        44185    44867     +682     
  Branches       354      356       +2     
===========================================
+ Hits         27399    27821     +422     
- Misses       16786    17046     +260

Flag	Coverage Δ
app	`48.90% <60.49%> (+0.27%)`	⬆️
python	`99.43% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ackages/components/src/components/Popout/index.tsx	`33.33% <0.00%> (ø)`
app/packages/looker/src/overlays/heatmap.ts	`16.06% <0.00%> (ø)`
app/packages/state/src/recoil/looker.ts	`29.70% <0.00%> (-0.61%)`	⬇️
app/packages/looker/src/overlays/base.ts	`39.15% <2.32%> (-10.09%)`	⬇️
.../packages/state/src/hooks/useSessionColorScheme.ts	`20.48% <20.48%> (ø)`
app/packages/state/src/recoil/color.ts	`43.06% <23.91%> (-9.02%)`	⬇️
app/packages/looker/src/elements/common/tags.ts	`15.62% <25.00%> (+3.24%)`	⬆️
...ages/state/src/hooks/useClearSessionColorScheme.ts	`30.18% <30.18%> (ø)`
app/packages/looker/src/elements/common/util.ts	`24.26% <30.76%> (+2.17%)`	⬆️
app/packages/state/src/hooks/useStateUpdate.ts	`28.42% <31.03%> (+0.35%)`	⬆️
... and 17 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

manivoxel51

nice work Lanny! leaving some thoughts - still reviewing / testing

app/packages/app/src/Network.tsx

app/yarn.lock

app/packages/core/src/components/Actions/Options.tsx

app/packages/core/src/components/ColorModal/ColorModal.tsx

benjaminpkane

Looks really good! Still playing with functionality. We can sync up today to talk backend details

app/package.json

app/packages/app/src/Network.tsx

app/packages/core/src/components/Actions/Options.tsx

app/packages/core/src/components/ColorModal/ColorModal.tsx

app/packages/core/src/components/Sidebar/Sidebar.tsx

app/packages/core/src/components/ColorModal/ColorModal.tsx

manivoxel51

nice work @lanzhenw ! 🍨

The code is looking good! nice work separating the logic between different options in color settings 💯

it looks reasonably safe and isolated. Let's merge this if

basic app features are working as expected - for example, you can open sample modal, tag, navigate between them, filtering, visualization, etc.

for followup: One trend I noticed was filtering or mapping over collections and not caching them. This can cause performance issues if heavy calculations run for every field every re-render. We can avoid some these maybe by adding useMemos, useCallbacks, etc. to avoid re-defining these values/functions on re-renders

manivoxel51 · 2023-04-28T14:45:25Z

app/packages/core/src/components/ColorModal/ColorFooter.tsx

@@ -51,6 +58,11 @@ const ColorFooter: React.FC<Prop> = ({ eligibleFields }) => {
    onCancel();
  };

+  const onClearSave = () => {


nit: useCallback cache the result on re-render

manivoxel51 · 2023-04-28T14:49:50Z

app/packages/state/src/hooks/useClearSessionColorScheme.ts

+import useSendEvent from "./useSendEvent";
+import { DEFAULT_APP_COLOR_SCHEME } from "../utils";
+
+const useClearSessionColorScheme = () => {


nit: could combine this class with useClearSavedColorScheme - There are similarities

Implemented.

app/packages/core/src/components/Actions/Options.tsx

manivoxel51 · 2023-04-28T15:05:16Z

app/packages/core/src/components/ColorModal/ColorModal.tsx

+  const videoFields = useRecoilValue(
+    fos.fields({
+      space: fos.State.SPACE.FRAME,
+      ftype: EMBEDDED_DOCUMENT_FIELD,
+    })
+  )?.filter((f) =>
+    VALID_LABEL_TYPES.includes(
+      f?.embeddedDocType?.split(".")?.slice(-1)[0] ?? ""
+    )
+  );
+  const sampleFields = useRecoilValue(
+    fos.fields({
+      space: fos.State.SPACE.SAMPLE,
+      ftype: EMBEDDED_DOCUMENT_FIELD,
+    })
+  )?.filter((f) =>
+    VALID_LABEL_TYPES.includes(
+      f?.embeddedDocType?.split(".")?.slice(-1)[0] ?? ""
+    )
+  );
+  const customizeColorFields = [...videoFields, ...sampleFields];


nit: for performance, we could cache these results by using useMemo or similar way

I made a eligibleFieldsToCustomizeColor recoil selector.

manivoxel51 · 2023-04-28T15:08:25Z

app/packages/core/src/components/ColorModal/FieldSetting.tsx

+  const defaultColor =
+    coloring.pool[Math.floor(Math.random() * coloring.pool.length)];
+  const expandedPath = useRecoilValue(fos.expandPath(path!));
+  const VALID_COLOR_ATTRIBUTE_TYPES = [BOOLEAN_FIELD, INT_FIELD, STRING_FIELD];


nit: move this to a constant outside

manivoxel51 · 2023-04-28T15:24:00Z

app/packages/core/src/components/FieldLabelAndInfo/index.tsx

@@ -273,6 +293,13 @@ function FieldInfoExpanded({
      onClick={(e) => e.stopPropagation()}
    >
      <FieldInfoExpandedContainer color={color}>
+        {field.embeddedDocType && !isModal && canEdit && (


this is basically one of the few places we are adding our feature 👍

yes, this is the place in the sidebar. I am moving canEdit flag to the saving part.

manivoxel51 · 2023-04-28T15:25:52Z

app/packages/looker/src/elements/common/tags.ts

@@ -426,3 +423,58 @@ const getFieldAndValue = (

  return [field, value, list];
 };
+
+const compareObjectArrays = (arr1, arr2) => {


nit: can you use loadash for these?

the lodash isEqual function is strict with the order of the array. These two helper functions only check if elements are the same, regardless of order.

manivoxel51 · 2023-04-28T15:26:08Z

app/packages/looker/src/elements/common/tags.ts

+};
+
+// Helper function to sort arrays of objects based on their key-value pairs
+function sortObjectArrays(a, b) {


nit: same here

manivoxel51 · 2023-04-28T15:27:35Z

app/packages/looker/src/overlays/base.ts

+        const fieldColor = customizeColorSetting.find(
+          (s) => s.field === this.field
+        )?.fieldColor;


nit: I am unfamiliar with this code. hopefully the computation here is small enough not to impact rendering performance

manivoxel51 · 2023-04-28T15:32:35Z

fiftyone/server/mutation.py

+            view._dataset.save()
+            state.view = view
+
+        await dispatch_event(subscription, StateUpdate(state=state))


you might have to dispatch if save_to_app if you are following save view approach

lanzhenw added feature Work on a feature request app Issues related to App features labels Mar 21, 2023

lanzhenw requested a review from manivoxel51 March 21, 2023 17:04

lanzhenw self-assigned this Mar 21, 2023

lanzhenw force-pushed the feature/colorUpdate branch from a7fab9d to 048f882 Compare March 24, 2023 16:24

lanzhenw force-pushed the feature/colorUpdate branch 2 times, most recently from d5aee66 to f98d0e8 Compare March 28, 2023 21:54

lanzhenw force-pushed the feature/colorUpdate branch 4 times, most recently from 32dec3c to 677f5a1 Compare April 9, 2023 19:09

lanzhenw requested a review from benjaminpkane April 11, 2023 16:55

manivoxel51 reviewed Apr 14, 2023

View reviewed changes

benjaminpkane reviewed Apr 14, 2023

View reviewed changes

lanzhenw force-pushed the feature/colorUpdate branch 4 times, most recently from 885adfd to d9ce018 Compare April 24, 2023 19:30

lanzhenw force-pushed the feature/colorUpdate branch 5 times, most recently from f6fdb45 to 973876b Compare April 27, 2023 20:39

lanzhenw changed the title ~~[WIP] Color Customization~~ Color Customization V1 Apr 27, 2023

lanzhenw requested review from benjaminpkane, manivoxel51 and kaixi-wang April 27, 2023 20:42

lanzhenw marked this pull request as ready for review April 27, 2023 20:43

manivoxel51 approved these changes Apr 28, 2023

View reviewed changes

lanzhenw and others added 25 commits May 5, 2023 14:22

replace atoms

156dad8

fix issues and name consistency

f5bc0fb

serialization from sdk -> session

096b5d4

displayed session settings in UI

5fb7aa1

display color pallete as session changes

b338885

colors

0853642

color pallete sync with session

d567755

save and load colorScheme to dataset.appConfig.colorScheme

94ae515

tweak in loading colorScheme based on rules

dde3b3b

apply buttons

8c61558

clean up

5d7cc31

remove stray logs

a7ea303

add clear settings

3cef817

combine userClear hooks

344146e

apply changes to session instantly, refactor and address PR comments

4f35dc3

fix bugs

8ac5ed9

fix json viewer sync issue

2a7d459

fix bug

d2598d9

load session colorScheme and load dataset appconfig setting seperately

b45e190

trying to fix colorModal not showing up in teams

ec09b9e

attributeForColor, shuffle color, and other tweaks

6ff5ce3

ui tweak and fix session issues

572dc92

tweak opacity

d96a72d

tweak

7387aea

remove logs

d6f139e

lanzhenw force-pushed the feature/colorUpdate branch from 166160f to d6f139e Compare May 5, 2023 19:26

lanzhenw added 2 commits May 5, 2023 14:28

replace throw error in looker with a warning

8d31647

fix bug

fe19f63

lanzhenw merged commit 46a1952 into develop May 6, 2023

lanzhenw deleted the feature/colorUpdate branch May 6, 2023 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color Customization V1 #2824

Color Customization V1 #2824

lanzhenw commented Mar 21, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading

manivoxel51 left a comment

benjaminpkane left a comment

manivoxel51 left a comment

manivoxel51 Apr 28, 2023

manivoxel51 Apr 28, 2023

lanzhenw May 1, 2023

manivoxel51 Apr 28, 2023

lanzhenw May 1, 2023

manivoxel51 Apr 28, 2023

manivoxel51 Apr 28, 2023

lanzhenw May 1, 2023

manivoxel51 Apr 28, 2023

lanzhenw May 1, 2023

manivoxel51 Apr 28, 2023

manivoxel51 Apr 28, 2023

manivoxel51 Apr 28, 2023

Color Customization V1 #2824

Color Customization V1 #2824

Conversation

lanzhenw commented Mar 21, 2023 • edited Loading

How to test it?

What changes are proposed in this pull request?

How is this patch tested? If it is not, please explain why.

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

What areas of FiftyOne does this PR affect?

codecov bot commented Mar 24, 2023 • edited Loading

Codecov Report

manivoxel51 left a comment

Choose a reason for hiding this comment

benjaminpkane left a comment

Choose a reason for hiding this comment

manivoxel51 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanzhenw commented Mar 21, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading