Dynamic groups #2934

sashankaryal · 2023-04-26T21:58:56Z

What changes are proposed in this pull request?

Dynamic groups app support (backend tracked by Dynamic groups #2475)
Group code modularization and some cleanup

Notes to the reviewer:

Pay special attention to files in the app/packages/core/src/components/Modal/Group/DynamicGroup directory

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

Dataset for testing was generated using the following snippet. Every th samples are folded into a group (here, 25).

import fiftyone as fo
import fiftyone.zoo as foz
from fiftyone import ViewField as F

dynamic_groups = foz.load_zoo_dataset("quickstart-groups").clone()

every_th_sample = 25
# for each slice, for every th samples, assign an arbitrary "scene_id"
for slice in dynamic_groups.group_slices:
    dynamic_groups.group_slice = slice
    scene_id_counter = 0
    order_by_counter = 0
    for sample in dynamic_groups:
        sample.set_field("scene_id", scene_id_counter // every_th_sample)
        sample.set_field("timestamp", order_by_counter % every_th_sample)
        sample.save()
        scene_id_counter = scene_id_counter + 1
        order_by_counter = order_by_counter + 1
dynamic_groups.save()

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-04-26T22:09:10Z

Codecov Report

Patch coverage: 78.83% and project coverage change: -36.77 ⚠️

Comparison is base (70e5b77) 99.44% compared to head (4b20a9e) 62.67%.

❗ Current head 4b20a9e differs from pull request most recent head a0f20c8. Consider uploading reports for the commit a0f20c8 to get more accurate results

Additional details and impacted files

@@                     Coverage Diff                     @@
##           feature/dynamic-groups    #2934       +/-   ##
===========================================================
- Coverage                   99.44%   62.67%   -36.77%     
===========================================================
  Files                          22      263      +241     
  Lines                       11843    45289    +33446     
  Branches                        0      353      +353     
===========================================================
+ Hits                        11777    28385    +16608     
- Misses                         66    16904    +16838

Flag	Coverage Δ
app	`49.65% <78.17%> (?)`
python	`99.44% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
.../components/src/components/TabOption/TabOption.tsx	`46.22% <23.07%> (ø)`
app/packages/state/src/recoil/utils.ts	`22.72% <25.00%> (ø)`
app/packages/state/src/recoil/view.ts	`55.73% <26.80%> (ø)`
app/packages/state/src/recoil/atoms.ts	`83.72% <28.76%> (ø)`
app/packages/state/src/recoil/aggregations.ts	`34.18% <35.48%> (ø)`
app/packages/state/src/hooks/useStateUpdate.ts	`27.84% <44.44%> (ø)`
app/packages/state/src/recoil/groups.ts	`49.52% <47.64%> (ø)`
app/packages/state/src/recoil/modal.ts	`58.33% <50.00%> (ø)`
.../paginateDynamicGroupSampleIdsPageQuery.graphql.ts	`100.00% <100.00%> (ø)`
...ed__/paginateDynamicGroupSampleIdsQuery.graphql.ts	`100.00% <100.00%> (ø)`
... and 8 more

... and 225 files with indirect coverage changes

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

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

fiftyone/server/samples.py

Co-authored-by: Benjamin Kane <[email protected]>

benjaminpkane

LGTM!

benjaminpkane · 2023-05-08T19:47:13Z

app/packages/core/src/components/Modal/Group/GroupView.tsx

+
+        {!shouldSplitVertically && is3DVisible && <GroupSample3d />}
+      </div>
+      {subBar ?? subBar}


I don't use nullish coalescing very often, so I may be missing something, but this seems like it has no effect

Meant to do &&

benjaminpkane · 2023-05-08T19:49:42Z

app/packages/flashlight/src/index.ts

    this.element.classList.add(flashlight);
    this.state = this.getEmptyState(config);

    document.addEventListener("visibilitychange", () => this.render());

+    if (config.enableKeyNavigation) {


I may update this in my current grid work, looks good for the feature. But it looks horizontal mode specific, and I think document keyboard event listeners can cause regressions

I'll add a guard for horizontal nav

benjaminpkane · 2023-05-08T19:50:15Z

app/packages/flashlight/src/index.ts

+        if (e.key === config.enableKeyNavigation.previousKey) {
+          e.preventDefault();
+          config.enableKeyNavigation.navigationCallback(true);
+          this.container.scrollLeft -= 316;


Is this arbitrary? A comment may help

benjaminpkane · 2023-05-08T19:52:43Z

app/packages/state/src/recoil/aggregations.ts

+      paths,
+      root = false,
+      mixed = false,
+      customView = undefined,


Could be added to the variables type with a union

benjaminpkane · 2023-05-08T19:53:05Z

app/packages/state/src/recoil/aggregations.ts

+    ({ get }) => {
+      const aggregations = get(
+        aggregationQuery({
+          customView: get(viewAtoms.dynamicGroupViewQuery(groupByValue)),


type error from above

app/packages/state/src/recoil/groups.ts

benjaminpkane · 2023-05-08T20:00:43Z

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

  const groupStats = useRecoilValue(groupStatistics(false));

+  const shouldShowSliceSelector = useMemo(
+    () => isGroup && groupSlices.length > 0,


Could be wrong, but should it be > 1?

benjaminpkane

LGTM!

sashankaryal · 2023-05-08T20:57:12Z

LGTM!

Thanks for leaving the comments. Will address comments in another PR.

sashankaryal requested a review from a team April 26, 2023 21:59

sashankaryal changed the base branch from develop to feature/dynamic-groups April 26, 2023 22:00

sashankaryal force-pushed the feat/dynamic-groups-app branch from db54811 to 4a7f7d5 Compare May 4, 2023 17:34

sashankaryal changed the title ~~[WIP] Dynamic groups~~ Dynamic groups May 5, 2023

sashankaryal self-assigned this May 5, 2023

sashankaryal added 19 commits May 5, 2023 10:40

unordered dynamic groups mvp

7b8045e

temp ideation for auto scrolling flashlight

7632556

show group slice selector is media type is group

9bc4a41

modularize group

dafce2c

add group context provider

a24173b

move css to group dir

1fd2e1d

add pagination

cb51484

prefetch

4a718c7

use selector instead of input for selecting dynamic group params

6d7ccd1

use selector instead of input for selecting dynamic group params

3d3f8b9

another pass for pagination

1bc648f

add reset button

4e5758f

cleanup

54e469c

random access works

6588575

accept style as prop for loadingDots

32a015e

add tooltip that shows orderby key

7e4265c

add keyboard navigation

b4303b5

use default icons

aae3af4

remove superfluous files

91dc34d

benjaminpkane reviewed May 8, 2023

View reviewed changes

app/packages/core/src/components/Actions/DynamicGroupAction.tsx Outdated Show resolved Hide resolved

benjaminpkane reviewed May 8, 2023

View reviewed changes

fiftyone/server/samples.py Outdated Show resolved Hide resolved

sashankaryal force-pushed the feat/dynamic-groups-app branch from ce35fe6 to 91dc34d Compare May 8, 2023 17:37

sashankaryal and others added 2 commits May 8, 2023 13:22

use react callback instead of recoil callback

069f512

Co-authored-by: Benjamin Kane <[email protected]>

remove file logging

bdc0393

sashankaryal requested a review from a team May 8, 2023 18:26

use constant

a0f20c8

benjaminpkane approved these changes May 8, 2023

View reviewed changes

sashankaryal merged commit 5349f0f into feature/dynamic-groups May 8, 2023

sashankaryal deleted the feat/dynamic-groups-app branch May 8, 2023 20:57

sashankaryal mentioned this pull request May 10, 2023

Address dynamic groups PR comments #3000

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic groups #2934

Dynamic groups #2934

sashankaryal commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

benjaminpkane left a comment

benjaminpkane May 8, 2023

sashankaryal May 10, 2023

benjaminpkane May 8, 2023

sashankaryal May 10, 2023

benjaminpkane May 8, 2023

benjaminpkane May 8, 2023

benjaminpkane May 8, 2023

benjaminpkane May 8, 2023

benjaminpkane left a comment

sashankaryal commented May 8, 2023

Dynamic groups #2934

Dynamic groups #2934

Conversation

sashankaryal commented Apr 26, 2023 • edited Loading

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 Apr 26, 2023 • edited Loading

Codecov Report

benjaminpkane 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

benjaminpkane left a comment

Choose a reason for hiding this comment

sashankaryal commented May 8, 2023

sashankaryal commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading