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

Scatterplot fixes and improvements #11898

Merged
merged 13 commits into from
Jan 3, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Pattern } from '@/util/ast/match'
import { getTextWidthBySizeAndFamily } from '@/util/measurement'
import { defineKeybinds } from '@/util/visualizationBuiltins'
import { computed, ref, watch, watchEffect, watchPostEffect } from 'vue'
import { ToolbarItem } from './toolbar'

export const name = 'Scatter Plot'
export const icon = 'points'
Expand Down Expand Up @@ -707,12 +708,12 @@ watchPostEffect(() => {
const yScale_ = yScale.value
const plotData = getPlotData(data.value) as Point[]
const series = Object.keys(data.value.axis).filter((s) => s != 'x')
const colorScale = (d: string) => {
const colorScale = (d: Point) => {
const color = d3.scaleOrdinal(d3.schemeCategory10).domain(series)
if (data.value.is_multi_series) {
return color(d)
return color(d.series ?? '')
}
return DEFAULT_FILL_COLOR
return d.color ?? DEFAULT_FILL_COLOR
}
d3Points.value
.selectAll<SVGPathElement, unknown>('path')
Expand All @@ -730,7 +731,7 @@ watchPostEffect(() => {
'd',
symbol.type(matchShape).size((d) => (d.size ?? 0.15) * SIZE_SCALE_MULTIPLER),
)
.style('fill', (d) => colorScale(d.series || ''))
.style('fill', (d) => colorScale(d))
.attr('transform', (d) => `translate(${xScale_(Number(d.x))}, ${yScale_(d.y)})`)
if (data.value.points.labels === VISIBLE_POINTS) {
d3Points.value
Expand Down Expand Up @@ -865,42 +866,49 @@ const makeSeriesLabelOptions = () => {
return seriesOptions
}

config.setToolbar([
{
icon: 'select',
title: 'Enable Selection',
toggle: selectionEnabled,
},
{
icon: 'show_all',
title: 'Fit All',
onClick: () => zoomToSelected(false),
},
{
icon: 'zoom',
title: 'Zoom to Selected',
disabled: () => brushExtent.value == null,
onClick: zoomToSelected,
},
{
icon: 'add_to_graph_editor',
title: 'Create component of selected points',
disabled: () => !createNewFilterNodeEnabled.value,
onClick: createNewFilterNode,
},
{
type: 'textSelectionMenu',
selectedTextOption: yAxisSelected,
title: 'Choose Y Axis Label',
heading: 'Y Axis Label: ',
options: {
none: {
label: 'No Label',
},
...makeSeriesLabelOptions(),
const createTextSelectionButton = (): ToolbarItem => ({
type: 'textSelectionMenu',
selectedTextOption: yAxisSelected,
title: 'Choose Y Axis Label',
heading: 'Y Axis Label: ',
options: {
none: {
label: 'No Label',
},
...makeSeriesLabelOptions(),
},
])
})

function useScatterplotVizToolbar() {
const textSelectionButton = createTextSelectionButton()
return computed(() => [
{
icon: 'select',
title: 'Enable Selection',
toggle: selectionEnabled,
},
{
icon: 'show_all',
title: 'Fit All',
onClick: () => zoomToSelected(false),
},
{
icon: 'zoom',
title: 'Zoom to Selected',
disabled: () => brushExtent.value == null,
onClick: zoomToSelected,
},
{
icon: 'add_to_graph_editor',
title: 'Create component of selected points',
disabled: () => !createNewFilterNodeEnabled.value,
onClick: createNewFilterNode,
},
...(data.value.is_multi_series ? [textSelectionButton] : []),
])
}

config.setToolbar(useScatterplotVizToolbar())
</script>

<template>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ type Point_Data
## PRIVATE
Row_Number

## PRIVATE
Colour
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like having both constructors Color and Colour is quite confusing. Which one should I use? Are they equivalent? Looking at the code below:

        Point_Data.Color ->
            colour_col = Point_Data.Colour.lookup_in table

before I noticed there's 2 different constructors I was pretty sure this was a typo (once Color and once Colour). It's so so easy to confuse the two.

Wouldn't it be much cleaner to keep a single constructor but modify the lookup/fallback mechanism to accept columns with both names? E.g. we can keep only the Color constructor but allow it to detect columns named color as well as colour. I would very much prefer such solution if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree it isn't clear, I'll look into how we can see if it will detect both!


## PRIVATE

Returns all recognized point data fields.
Expand Down Expand Up @@ -94,6 +97,10 @@ type Point_Data
candidates.find if_missing=candidates.first is_good
Point_Data.Row_Number ->
Point_Data.iota table.row_count
Point_Data.Color ->
colour_col = Point_Data.Colour.lookup_in table
is_good c = (self.is_recognized c).not
if is_good colour_col then colour_col else Error.throw Nothing

_ -> Error.throw No_Fallback_Column

Expand Down
15 changes: 15 additions & 0 deletions test/Visualization_Tests/src/Scatter_Plot_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@ add_specs suite_builder =
table = Table.from_rows header [row_1, row_2]
expect table (labels index 'y') '[{"size":40,"x":0,"y":10, "row_number": 0},{"size":50,"x":1,"y":20, "row_number": 1}]' '"get_row"'

group_builder.specify "recognises color and colour for the colour column, colour" <|
header = [ 'u' , 'k' , 'size', 'colour']
row_1 = [ 10 , 3 , 40, 'black']
row_2 = [ 20 , 4 , 50, 'white']
table = Table.from_rows header [row_1, row_2]
expect table (labels 'u' 'k') '[{"x":10,"y":3,"color":"black","size":40,"row_number":0},{"x":20,"y":4,"color":"white","size":50,"row_number":1}]' '"get_row"'

group_builder.specify "recognizes color and colour for the colour column, color" <|
header = [ 'u' , 's' , 'size', 'color']
row_1 = [ 10 , 3 , 40, 'red']
row_2 = [ 20 , 4 , 50, 'white']
row_3 = [ 30 , 5 , 50, 'blue']
table = Table.from_rows header [row_1, row_2, row_3]
expect table (labels 'u' 's') '[{"x":10,"y":3,"color":"red", "size":40,"row_number": 0},{"x":20,"y":4,"color":"white", "size":50,"row_number": 1}, {"x":30,"y":5,"color":"blue", "size":50,"row_number": 2}]' '"get_row"'

group_builder.specify "using indices for x if given a vector" <|
vector = [0,10,20]
expect vector no_labels '[{"x":0,"y":0, "row_number": 0},{"x":1,"y":10, "row_number": 1},{"x":2,"y":20, "row_number": 2}]' '"at"'
Expand Down
Loading