Bubble markers map type #257

moontrip · 2023-05-25T12:55:01Z

(Connor is new lead on this PR)
Resolves #263
Resolves #95
Resolves #249

chowington · 2023-06-20T19:39:20Z

Numbers in the bubbles are just for sanity checking---not meant to be in the final product!

moontrip · 2023-06-20T19:47:23Z

The screenshot looks great, @chowington 👍 Seems like center label helps to alleviate overlapped bubbles like bottom left bubbles in your screenshot.

d-callan · 2023-06-21T14:33:20Z

we do need a way for people to read the values, and its possible labels on the bubbles will prove the best way to do that. my only concern was about maintaining enough contrast to be readable against all hues in our gradient colormap. we can maybe try it though and see what it looks like.

packages/libs/components/src/map/BubbleMarker.tsx

d-callan · 2023-06-21T14:42:11Z

packages/libs/components/src/stories/BubbleMarkers.stories.tsx

+const valueToSizeMapper = (value: number) => {
+  // Area scales directly with value
+  const constant = 100;
+  const area = value * constant;


so i can live w area based scaling if we include a note in the legend that thats how weve done it.

d-callan · 2023-06-21T14:45:07Z

packages/libs/components/src/stories/BubbleMarkers.stories.tsx

+
+const valueToSizeMapper = (value: number) => {
+  // Area scales directly with value
+  const constant = 100;


can you explain to me? is this to do w very small values or something?

if so, we should shift the range maybe instead? add as opposed to multiply?

No, this is just the scaling factor/"slope" to convert a value to a size in pixels

i still dont understand..

It doesn't make sense to make the circle size exactly equal to the value, like a value of 500 -> 500px circle, so the constant is what you multiple the value by to get the circle size

I've changed it to scalingFactor locally, with I think is more clear

packages/libs/components/src/stories/plotControls/PlotLegend.stories.tsx

d-callan · 2023-06-21T14:51:00Z

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx

@@ -102,6 +111,7 @@ enum MapSideNavItemLabels {
 enum MarkerTypeLabels {
  pie = 'Donuts',
  barplot = 'Bar plots',
+  bubble = 'Bubbles',


this reminds me we need to discuss whether we can put bubble map before the donuts and have that be default in a ux meeting..

d-callan · 2023-06-21T14:53:35Z

packages/libs/eda/src/lib/map/analysis/MarkerConfiguration/BubbleMarkerConfigurationMenu.tsx

+export interface BubbleMarkerConfiguration
+  extends MarkerConfiguration<'bubble'> {
+  selectedVariable: VariableDescriptor;
+  selectedValues: string[] | undefined;


if i understand this right i think this works for cat vars. cont need to choose an aggregation method.

d-callan · 2023-06-21T14:55:15Z

packages/libs/eda/src/lib/map/analysis/appState.ts

+          {
+            type: 'bubble',
+            selectedVariable: defaultVariable,
+            selectedValues: undefined,


so this is maybe interesting.. if the first featured var is categorical and we have no default values selected does the user see anything in bubble mode without having to configure something?

I don't think we've got as far as handling the selected values yet.

d-callan · 2023-06-21T14:59:04Z

packages/libs/eda/src/lib/map/analysis/hooks/standaloneMapMarkers.tsx

+      : 0;
+
+    const bubbleValueToSizeMapper = (value: number) => {
+      const largestCircleArea = 9000;


how did we decide this?

I eyeballed it 😉

d-callan · 2023-06-21T15:02:22Z

packages/libs/eda/src/lib/map/analysis/hooks/standaloneMapMarkers.tsx

@@ -316,30 +368,50 @@ export function useStandaloneMapMarkers(
                },
              ];

+        // here's what Bob pointed out
        const count =
          vocabulary != null // if there's an overlay (all expected use cases)
            ? overlayValues.reduce((sum, { count }) => (sum = sum + count), 0)


just curious, why not just use entityCount always?

Dunno, I just left this as it was!

d-callan · 2023-06-21T15:03:42Z

packages/libs/eda/src/lib/map/analysis/hooks/standaloneMapMarkers.tsx

+            label: reorderedData[0].label,
+            value: count,
+            color:
+              'color' in reorderedData[0] ? reorderedData[0].color : undefined,


is this a temp thing until i have the backend put together? not sure i understand..

Not sure what in particular you're talking about here, but yeah most likely a temporary thing, lol

asizemore · 2023-06-22T09:38:15Z

#311 for the bubble icon

For the overlapping bubbles, it's common in bubble charts to have the stroke be whatever color and then the fill be the same color but maybe 0.7 opacity like in the following

And sorry to leave the discussion early at the dev meeting yesterday! Did y'all come to a conclusion about the bubble sizing wrt different zoom levels and the legend?

bobular

I just want to put these comments in before I get distracted by other work. Will return for a proper review!

bobular · 2023-06-22T10:56:56Z

packages/libs/eda/src/lib/map/analysis/appState.ts

@@ -110,6 +115,11 @@ export function useAppState(uiStateKey: string, analysisState: AnalysisState) {
            selectedVariable: defaultVariable,
            selectedValues: undefined,
          },
+          {


The problem with old analyses not being backwards compatible will require some extra work here.

The problem was that appState.markerConfigurations was missing an entry for type: 'bubble' because the default configuration is currently only made when there is absolutely no configuration.

A solution (not sure if solves all future compatibility issues, but will solve this one for now) could be as follows:

Modify this particular useEffect to initialise appState.markerConfigurations to an empty array. Move the default markerConfigurations array outside here as defaultMarkerConfigurations.

Add a new useEffect directly after this one to check that appState.markerConfigurations contains the same number of elements as defaultMarkerConfigurations, and those elements have the same type values. If any are missing, add the appropriate element from defaultMarkerConfigurations. If there are any extra (future deprecated?) elements - probably remove them??

bobular · 2023-06-22T10:58:42Z

packages/libs/eda/src/lib/map/analysis/appState.ts

+          {
+            type: 'bubble',
+            selectedVariable: defaultVariable,
+            selectedValues: undefined,


I don't think we've got as far as handling the selected values yet.

bobular · 2023-06-22T16:29:54Z

@chowington are you planning to use dependentAxisRange in the bubble markers?
Probably not needed if there's a valueToSizeMapper function passed around?

If not, could you remove it please.

Also a switch statement here would be lovely please:

  const markers = useMemo(
    () =>
      markersData?.map((markerProps) =>
        markerType === 'pie' ? (
          <DonutMarkerComponent {...markerProps} />
        ) : markerType === 'bubble' ? (
          <BubbleMarkerComponent {...(markerProps as BubbleMarkerProps)} />
        ) : (
          <ChartMarkerComponent {...markerProps} />
        )
      ) || [],
    [markersData, markerType]
  );

bobular · 2023-06-22T16:46:19Z

Thinking more about accommodating the new back end.

In useStandaloneMapMarkers (should be renamed maybe to useStandaloneMapMarkerData?):

I think renaming this type StandaloneMapMarkersResponse into DonutMarkersResponse | ChartMarkersResponse with those two types actually being identical/aliases (until we tackle the tech debt mentioned above) - this will help with readability (I much prefer it to DonutAndChartMarkersResponse). And something similar with the request types.

Then we can move ahead with DonutMarkersResponse | ChartMarkersResponse | BubbleMarkersResponse.

The reduce in the same hook that returns { valueMax, valueMinPos, countSum } will likely break for the bubble response, but we only need to do it for the chart markers (hence my question above) - so we can start being a bit more selective about what we do for the different marker types.

chowington · 2023-08-02T20:37:07Z

Looks like I can't remove people as reviewers if they've already given a review--I'm assigning @bobular to be the primary reviewer if that's alright with you, Bob!

chowington · 2023-08-02T20:42:21Z

So honestly there's not much I can think of that requires specific attention/decision. Maybe some minor points:

I removed the numbers inside the bubbles since adding the popup, as they were not part of the mockup, and it would be a headache to make them fit inside the smaller bubbles well.
The default gradient is the brownish color; I don't particularly like that for the map, as it can be hard to distinguish the brown from the map background. That is the default we use for the scatter plot though. I may look into changing it unless others think it's fine.
There is still the issue with binary variables failing at the backend.

chowington · 2023-08-02T21:00:46Z

Actually, I'll tag @asizemore for the gradient topic, as she put a lot of thought into choosing these gradients!

chowington · 2023-08-04T18:18:34Z

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx


  const markers = useMemo(
    () =>
      markersData?.map((markerProps) =>
        markerType === 'pie' ? (
-          <DonutMarkerComponent {...markerProps} />


Remove type assertions, replace by prop filtering

chowington · 2023-08-04T18:19:22Z

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx

+                    overlayConfiguration={
+                      activeOverlayConfig.value as OverlayConfig
+                    }


Look into this assertion too

asizemore · 2023-08-07T10:25:55Z

So exciting to see this all come together!!! I'd be happy to tackle the gradient issue. I'll make it a new issue and assign myself

dmfalke

Connor and I did a synchronous review on Friday. The following issues will be addressed before merging:

Gradient legend issue: if min = max, then the gradient is empty (or white)
Fix y-axis config labels (proportion and aggregation are flipped)
Change tooltip label that is currently Color.

dmfalke

@chowington is looks like you've addressed the major things we discussed. I'm ready to approve this! Is there anything else you're working on, in this PR?

chowington · 2023-08-08T20:40:32Z

@dmfalke I'm just merging in main and testing, but I'm done otherwise 👍

Edit: Done

dmfalke

🚀

preliminary bubble marker component

3ff0400

moontrip linked an issue May 31, 2023 that may be closed by this pull request

Bubble marker react component #95

Closed

chowington added 7 commits June 8, 2023 12:58

Merge branch 'main' into 95-bubble-marker-component

2991c81

Update bubble spec and design

e739bb3

Merge branch 'main' into 95-bubble-marker-component

fc03c3e

Merge branch 'main' into 95-bubble-marker-component

c207973

Get working bubble markers on map

9306842

Merge branch 'main' into 95-bubble-marker-component

3a324ad

Add dedicated bubble marker menu option

96ad6e9

chowington self-assigned this Jun 14, 2023

chowington added 4 commits June 20, 2023 13:00

Make Bubble Marker Legend

0688eec

Merge branch 'main' into 95-bubble-marker-component

fb5e693

Add missing stuff related to bubble markers

d0a5f21

Add bubble marker legend to SAM

910286a

chowington changed the title ~~preliminary bubble marker component~~ Bubble markers map type Jun 20, 2023

chowington requested review from bobular, dmfalke and d-callan June 20, 2023 18:01

Set max bubble marker size

764efb2

d-callan reviewed Jun 21, 2023

View reviewed changes

bobular reviewed Jun 22, 2023

View reviewed changes

chowington added 3 commits June 26, 2023 16:55

Merge branch 'main' into 95-bubble-marker-component

e50b2e6

Add outline to bubble markers and update legend tick calculation

3d0902a

Fix bubble outline shape and draw smaller bubbles over bigger

29b79a5

chowington marked this pull request as ready for review August 2, 2023 20:37

chowington removed request for d-callan and dmfalke August 2, 2023 20:47

Remove clear button from bubble menu variable input

7db7cd3

chowington added 2 commits August 2, 2023 17:36

Use _.get() where possible to shorten expressions

4e41f15

Change === null to == null to prevent error

7d01ede

chowington requested review from dmfalke and removed request for bobular August 3, 2023 19:14

chowington reviewed Aug 4, 2023

View reviewed changes

asizemore mentioned this pull request Aug 7, 2023

Improve map gradient color #405

Closed

chowington added 2 commits August 7, 2023 09:56

Fix incorrect bubble config input labels

5618004

Use sequential colormap when data is sequential

cb44ae2

dmfalke reviewed Aug 7, 2023

View reviewed changes

chowington added 4 commits August 8, 2023 00:24

Refactor useStandaloneMapMarkers

aacff44

Fix issues when min and max bubble values are equal

835d820

Add rest of bubble popup overlay label logic

ef1580f

Simplify some bits of code

74cfc94

dmfalke reviewed Aug 8, 2023

View reviewed changes

Merge branch 'main' into 95-bubble-marker-component

f57339b

chowington requested a review from dmfalke August 8, 2023 20:41

Remove unnecessary bubble size short circuit

ff12879

dmfalke approved these changes Aug 9, 2023

View reviewed changes

chowington merged commit fcf36a4 into main Aug 9, 2023
1 check passed

chowington deleted the 95-bubble-marker-component branch August 9, 2023 13:08

Bubble markers map type #257

Bubble markers map type #257

Conversation

moontrip commented May 25, 2023 • edited by chowington Loading

chowington commented Jun 20, 2023 • edited Loading

moontrip commented Jun 20, 2023

d-callan commented Jun 21, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asizemore commented Jun 22, 2023

bobular left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobular commented Jun 22, 2023

bobular commented Jun 22, 2023

chowington commented Aug 2, 2023 • edited Loading

chowington commented Aug 2, 2023

chowington commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asizemore commented Aug 7, 2023

dmfalke left a comment

Choose a reason for hiding this comment

dmfalke left a comment

Choose a reason for hiding this comment

chowington commented Aug 8, 2023 • edited Loading

dmfalke left a comment

Choose a reason for hiding this comment

moontrip commented May 25, 2023 •

edited by chowington

Loading

chowington commented Jun 20, 2023 •

edited

Loading

chowington commented Aug 2, 2023 •

edited

Loading

chowington commented Aug 8, 2023 •

edited

Loading