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

Group modal and orthographic projection improvements #2759

Merged
merged 6 commits into from
Mar 10, 2023

Conversation

sashankaryal
Copy link
Contributor

@sashankaryal sashankaryal commented Mar 10, 2023

What changes are proposed in this pull request?

Known issues

  • Pinned slice persists in the looker when switching samples, but not in the sidebar. Not a regression.
    • It didn't use to persist in the looker as well, but that has been fixed.
  • Orthographic projection metadata, if calculated, is only available in the pointcloud slice. However, the sidebar shows this field for all slices. This can create a confusing experience. Not a regression.
  • Projected bounding boxes are painted off the center once in a blue moon-ly. I haven't been able to reproduce this but I'll dive deeper in next iteration. Not a regression.
  • Nit: Group dataset primitives and state management need some rethinking and refactoring. Atoms and selectors are scattered across multiple places and it's usually not clear how different states are related, or when certain state updates take place.

Regression Testing

  • Test with quickstart dataset.
  • Test with quickstart-groups dataset.
    • Without calculating orthographic projection, for each slice, expand a sample and navigate prev/next. Change pinned sample and navigate prev/next.
    • Repeat above but calculate orthographic projection first.

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

Locally. No automated testing.

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.

Refer to #2660

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

@sashankaryal sashankaryal changed the title Orthographic projection improvements Group Modal / Orthographic projection improvements Mar 10, 2023
@sashankaryal sashankaryal changed the title Group Modal / Orthographic projection improvements Group modal and orthographic projection improvements Mar 10, 2023
@sashankaryal sashankaryal self-assigned this Mar 10, 2023
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch coverage: 70.09% and project coverage change: +0.47 🎉

Comparison is base (02bb409) 61.40% compared to head (5d02e97) 61.87%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2759      +/-   ##
===========================================
+ Coverage    61.40%   61.87%   +0.47%     
===========================================
  Files          256      259       +3     
  Lines        42954    43528     +574     
  Branches       349      350       +1     
===========================================
+ Hits         26374    26934     +560     
- Misses       16580    16594      +14     
Flag Coverage Δ
app 48.65% <64.44%> (+0.23%) ⬆️
python 99.41% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
app/packages/looker/src/elements/common/looker.ts 9.24% <0.00%> (-0.24%) ⬇️
...nts/src/components/ErrorBoundary/ErrorBoundary.tsx 27.58% <12.50%> (+1.75%) ⬆️
app/packages/state/src/hooks/useCreateLooker.ts 17.00% <16.66%> (+0.22%) ⬆️
app/packages/looker/src/elements/common/actions.ts 33.53% <18.18%> (-0.11%) ⬇️
app/packages/state/src/recoil/groups.ts 52.27% <32.43%> (+0.31%) ⬆️
app/packages/state/src/recoil/modal.ts 58.33% <50.00%> (ø)
...ages/components/src/components/CodeBlock/index.tsx 100.00% <100.00%> (ø)
...ges/components/src/components/CopyButton/index.tsx 100.00% <100.00%> (ø)
.../components/src/components/ThemeProvider/index.tsx 91.71% <100.00%> (+0.16%) ⬆️
app/packages/components/src/components/index.ts 100.00% <100.00%> (ø)
... and 2 more

... and 25 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Co-authored-by: Benjamin Kane <ben@voxel51.com>
@sashankaryal sashankaryal merged commit 215ad2f into develop Mar 10, 2023
@sashankaryal sashankaryal deleted the bug/ortho-projection-fix branch March 10, 2023 06:41
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.

[FR] App should include point cloud slices in group selector when projection metadata is available
3 participants