-
Notifications
You must be signed in to change notification settings - Fork 595
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
Colorscheme1.1 Heatmap #3804
Colorscheme1.1 Heatmap #3804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defer to @manivoxel51 on approval. Code wise, this looks pretty complete! Any fixes could happen in separate PRs from my perspective
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #3804 +/- ##
===========================================
+ Coverage 15.63% 15.66% +0.03%
===========================================
Files 672 726 +54
Lines 77832 80365 +2533
Branches 1041 1069 +28
===========================================
+ Hits 12168 12593 +425
- Misses 65664 67772 +2108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!💥
Tested the app with heatmaps and didn't notice any blocking issues. I agree that we can crush any known bugs as a followup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lanzhenw, @benjaminpkane e2e is failing - can we get those fixed before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…orScaleList.tsx Co-authored-by: Benjamin Kane <[email protected]>
@manivoxel51 I fixed the e2e tests and the typing issue. typing 000 => 0 Screen.Recording.2023-11-20.at.8.22.06.AM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll investigate the failing test - locally passing - CI failing
As a user, I can configure the heatmap field and float field colorscale through either providing a named colorscale or manually defining a value color range list. The field will first look for field specific colorscale defined, and then check
defaultColorscale
of the session.Set colorscale for a field on UI:
global-field.mov
Set colorscale with manual defined list:
manual.list.mov
Set colorscale for a float type field to render the embeddings:
float_embeddings.mov
Example schema:
What changes are proposed in this pull request?
This PR also includes changes in the backend session workflow of how colorscheme is handled.
We added
id
field to the object (needs migration)How is this patch tested? If it is not, please explain why.
How to create a dataset with heatmap?
bugs:
Regression: session.color_scheme = fo.Colorscheme(color_by="field",
color_pool=["#ff0000", "#00ff00", "#0000ff"]) is not working.
Initial load: rgb is not loaded until we open the color panel and change certain settings
datasetColorscheme
fragment does not reliably update whensave as default
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
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?
fiftyone
Python library changes