Skip to content

Add a slider to resize plots vertically #3428

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.

Sign up for GitHub

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

Merged
sroy3 merged 10 commits into from
Mar 14, 2023
Merged

Add a slider to resize plots vertically #3428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d42a208
Add a slider to resize plots vertically
Mar 8, 2023
0301235
Merge branch 'main' in resize-plots-height
Mar 8, 2023
9949415
Add and fix tests
Mar 10, 2023
0973ba7
Remove duplication
Mar 10, 2023
a43fd7d
Merge branch 'main' into resize-plots-height
sroy3 Mar 10, 2023
48cf3d3
Fix test
Mar 10, 2023
0344324
Merge branch 'resize-plots-height' of https://github.com/iterative/vs…
Mar 10, 2023
b9994c5
Apply review comments
Mar 13, 2023
56e010d
Change how truncateVerticalTitles work
Mar 13, 2023
1ab4679
Merge branch 'main' into resize-plots-height
Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions extension/src/plots/model/collect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ const collectTemplatePlot = (
template: string,
revisionData: RevisionData,
nbItemsPerRow: number,
height: number,
revisionColors: ColorScale | undefined,
multiSourceEncoding: MultiSourceEncoding
) => {
Expand All @@ -608,6 +609,7 @@ const collectTemplatePlot = (
const content = extendVegaSpec(
fillTemplate(template, datapoints),
nbItemsPerRow,
height,
{
...multiSourceEncodingUpdate,
color: revisionColors
Expand All @@ -629,6 +631,7 @@ const collectTemplateGroup = (
templates: TemplateAccumulator,
revisionData: RevisionData,
nbItemsPerRow: number,
height: number,
revisionColors: ColorScale | undefined,
multiSourceEncoding: MultiSourceEncoding
): TemplatePlotEntry[] => {
Expand All @@ -647,6 +650,7 @@ const collectTemplateGroup = (
template,
revisionData,
nbItemsPerRow,
height,
revisionColors,
multiSourceEncoding
)
Expand All @@ -660,6 +664,7 @@ export const collectSelectedTemplatePlots = (
templates: TemplateAccumulator,
revisionData: RevisionData,
nbItemsPerRow: number,
height: number,
revisionColors: ColorScale | undefined,
multiSourceEncoding: MultiSourceEncoding
): TemplatePlotSection[] | undefined => {
Expand All @@ -672,6 +677,7 @@ export const collectSelectedTemplatePlots = (
templates,
revisionData,
nbItemsPerRow,
height,
revisionColors,
multiSourceEncoding
)
Expand Down
11 changes: 7 additions & 4 deletions extension/src/plots/model/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import {
SectionCollapsed,
CustomPlotData,
DEFAULT_HEIGHT,
DEFAULT_NB_ITEMS_PER_ROW
DEFAULT_NB_ITEMS_PER_ROW,
PlotHeight
} from '../webview/contract'
import {
ExperimentsOutput,
Expand Down Expand Up @@ -56,7 +57,7 @@ export class PlotsModel extends ModelWithPersistence {
private readonly experiments: Experiments

private nbItemsPerRow: Record<Section, number>
private height: Record<Section, number | undefined>
private height: Record<Section, PlotHeight>
private customPlotsOrder: CustomPlotsOrderValue[]
private sectionCollapsed: SectionCollapsed
private commitRevisions: Record<string, string> = {}
Expand Down Expand Up @@ -414,7 +415,7 @@ export class PlotsModel extends ModelWithPersistence {
return DEFAULT_NB_ITEMS_PER_ROW
}

public setHeight(section: Section, height: number | undefined) {
public setHeight(section: Section, height: PlotHeight) {
this.height[section] = height
this.persist(PersistenceKey.PLOT_HEIGHT, this.height)
}
Expand Down Expand Up @@ -510,7 +511,8 @@ export class PlotsModel extends ModelWithPersistence {
id,
title: truncateVerticalTitle(
id,
this.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)
this.getNbItemsPerRow(Section.CHECKPOINT_PLOTS),
this.getHeight(Section.CHECKPOINT_PLOTS)
) as string,
values: values.filter(value =>
selectedExperiments.includes(value.group)
Expand Down Expand Up @@ -562,6 +564,7 @@ export class PlotsModel extends ModelWithPersistence {
this.templates,
this.revisionData,
this.getNbItemsPerRow(Section.TEMPLATE_PLOTS),
this.getHeight(Section.TEMPLATE_PLOTS),
this.getRevisionColors(selectedRevisions),
this.multiSourceEncoding
)
Expand Down
48 changes: 31 additions & 17 deletions extension/src/plots/vega/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ import smoothTemplate from '../../test/fixtures/plotsDiff/templates/smooth'
import multiSourceTemplate from '../../test/fixtures/plotsDiff/templates/multiSource'
import { copyOriginalColors } from '../../experiments/model/status/colors'
import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
import { DEFAULT_NB_ITEMS_PER_ROW } from '../webview/contract'
import {
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT
} from '../webview/contract'

describe('isMultiViewPlot', () => {
it('should recognize the confusion matrix template as a multi view plot', () => {
Expand Down Expand Up @@ -86,7 +89,8 @@ describe('extendVegaSpec', () => {
it('should not add encoding if no color scale is provided', () => {
const extendedSpec = extendVegaSpec(
linearTemplate,
DEFAULT_NB_ITEMS_PER_ROW
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT
)
expect(extendedSpec.encoding).toBeUndefined()
})
Expand All @@ -99,6 +103,7 @@ describe('extendVegaSpec', () => {
const extendedSpec = extendVegaSpec(
linearTemplate,
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT,
{
color: colorScale
}
Expand Down Expand Up @@ -143,12 +148,12 @@ describe('extendVegaSpec', () => {

it('should truncate all titles from the left to 50 characters for large plots', () => {
const spec = withLongTemplatePlotTitle()
const updatedSpec = extendVegaSpec(spec, 1)
const updatedSpec = extendVegaSpec(spec, 1, DEFAULT_PLOT_HEIGHT)

const truncatedTitle = '…-many-many-characters-at-least-seventy-characters'
const truncatedHorizontalTitle =
'…any-many-characters-at-least-seventy-characters-x'
const truncatedVerticalTitle = '…racters-at-least-seventy-characters-y'
const truncatedVerticalTitle = '…acters-at-least-seventy-characters-y'

const specString = JSON.stringify(spec)
expect(specString).not.toContain(truncatedTitle)
Expand All @@ -169,12 +174,16 @@ describe('extendVegaSpec', () => {

it('should truncate all titles from the left to 50 characters for regular plots', () => {
const spec = withLongTemplatePlotTitle()
const updatedSpec = extendVegaSpec(spec, DEFAULT_NB_ITEMS_PER_ROW)
const updatedSpec = extendVegaSpec(
spec,
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT
)

const truncatedTitle = '…-many-many-characters-at-least-seventy-characters'
const truncatedHorizontalTitle =
'…any-many-characters-at-least-seventy-characters-x'
const truncatedVerticalTitle = '…racters-at-least-seventy-characters-y'
const truncatedVerticalTitle = '…rs-at-least-seventy-characters-y'

const specString = JSON.stringify(spec)
expect(specString).not.toContain(truncatedTitle)
Expand All @@ -195,11 +204,11 @@ describe('extendVegaSpec', () => {

it('should truncate all titles from the left to 30 characters for small plots', () => {
const spec = withLongTemplatePlotTitle()
const updatedSpec = extendVegaSpec(spec, 3)
const updatedSpec = extendVegaSpec(spec, 3, DEFAULT_PLOT_HEIGHT)

const truncatedTitle = '…s-at-least-seventy-characters'
const truncatedHorizontalTitle = '…at-least-seventy-characters-x'
const truncatedVerticalTitle = '…t-seventy-characters-y'
const truncatedVerticalTitle = '…at-least-seventy-characters-y'

const specString = JSON.stringify(spec)
expect(specString).not.toContain(truncatedTitle)
Expand All @@ -225,7 +234,7 @@ describe('extendVegaSpec', () => {
text: repeatedTitle
})

const updatedSpec = extendVegaSpec(spec, 3)
const updatedSpec = extendVegaSpec(spec, 3, DEFAULT_PLOT_HEIGHT)

const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890'

Expand All @@ -242,7 +251,7 @@ describe('extendVegaSpec', () => {
const repeatedTitle = 'abcdefghijklmnopqrstuvwyz1234567890'
const spec = withLongTemplatePlotTitle([repeatedTitle, repeatedTitle])

const updatedSpec = extendVegaSpec(spec, 3)
const updatedSpec = extendVegaSpec(spec, 3, DEFAULT_PLOT_HEIGHT)

const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890'

Expand All @@ -262,7 +271,7 @@ describe('extendVegaSpec', () => {
text: [repeatedTitle, repeatedTitle]
})

const updatedSpec = extendVegaSpec(spec, 3)
const updatedSpec = extendVegaSpec(spec, 3, DEFAULT_PLOT_HEIGHT)

const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890'

Expand All @@ -276,13 +285,18 @@ describe('extendVegaSpec', () => {
})

it('should update the multi-source template to remove erroneous shape encoding from the vertical line displayed on hover', () => {
const updatedSpec = extendVegaSpec(multiSourceTemplate, 1, {
color: { domain: [], range: [] },
shape: {
field: 'field',
scale: { domain: [], range: [] }
const updatedSpec = extendVegaSpec(
multiSourceTemplate,
1,
DEFAULT_PLOT_HEIGHT,
{
color: { domain: [], range: [] },
shape: {
field: 'field',
scale: { domain: [], range: [] }
}
}
})
)

expect(updatedSpec.encoding).not.toBeUndefined()
expect(updatedSpec.layer[1].layer[0].encoding.shape).toBeNull()
Expand Down
29 changes: 20 additions & 9 deletions extension/src/plots/vega/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,15 +253,19 @@ const truncateTitle = (
return titleCopy
}

export const truncateVerticalTitle = (title: Text | Title, size: number) =>
truncateTitle(title, (size > 2 ? 30 : 50) * 0.75)
export const truncateVerticalTitle = (
title: Text | Title,
width: number,
height: number
) => truncateTitle(title, Math.floor((50 - (width - height) * 5) * 0.75))

const isEndValue = (valueType: string) =>
['string', 'number', 'boolean'].includes(valueType)

export const truncateTitles = (
spec: TopLevelSpec,
size: number,
width: number,
height: number,
vertical?: boolean
// eslint-disable-next-line sonarjs/cognitive-complexity
) => {
Expand All @@ -281,16 +285,18 @@ export const truncateTitles = (
if (key === 'title') {
const title = value as unknown as Title
specCopy[key] = vertical
? truncateVerticalTitle(title, size)
: truncateTitle(title, size > DEFAULT_NB_ITEMS_PER_ROW ? 30 : 50)
? truncateVerticalTitle(title, width, height)
: truncateTitle(title, width > DEFAULT_NB_ITEMS_PER_ROW ? 30 : 50)
} else if (isEndValue(valueType)) {
specCopy[key] = value
} else if (Array.isArray(value)) {
specCopy[key] = value.map(val =>
isEndValue(typeof val) ? val : truncateTitles(val, size, vertical)
isEndValue(typeof val)
? val
: truncateTitles(val, width, height, vertical)
)
} else if (typeof value === 'object') {
specCopy[key] = truncateTitles(value, size, vertical)
specCopy[key] = truncateTitles(value, width, height, vertical)
}
}
return specCopy
Expand All @@ -300,14 +306,19 @@ export const truncateTitles = (

export const extendVegaSpec = (
spec: TopLevelSpec,
size: number,
width: number,
height: number,
encoding?: {
color?: ColorScale
strokeDash?: StrokeDashEncoding
shape?: ShapeEncoding
}
) => {
const updatedSpec = truncateTitles(spec, size) as unknown as TopLevelSpec
const updatedSpec = truncateTitles(
spec,
width,
height
) as unknown as TopLevelSpec

if (isMultiViewByCommitPlot(spec) || !encoding) {
return updatedSpec
Expand Down
28 changes: 19 additions & 9 deletions extension/src/plots/webview/contract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@ import { Color } from '../../experiments/model/status/colors'

export const DEFAULT_NB_ITEMS_PER_ROW = 2

export enum PlotHeight {
SMALLER = 0,
SMALL = 1,
REGULAR = 2,
SQUARE = 3,
VERTICAL_NORMAL = 4,
VERTICAL_LARGER = 5
Copy link
Contributor

@mattseddon mattseddon Mar 12, 2023

Choose a reason for hiding this comment

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

[Q] Could these be the aspect ratios if that is what they refer to?

Copy link
Contributor

@mattseddon mattseddon Mar 12, 2023

Choose a reason for hiding this comment

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

Would we gain/lose anything by having this as PlotAspectRatio?

Copy link
Contributor Author

@sroy3 sroy3 Mar 13, 2023

Choose a reason for hiding this comment

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

First iteration used aspect ratios instead, but it made it more complicated to hold fractions instead of numbers. It makes it way easier to change the aspect ratio at the end of the chain (stylesheet) than doing so inside the enum here.

}

export const DEFAULT_PLOT_HEIGHT = PlotHeight.SMALL

export enum Section {
CHECKPOINT_PLOTS = 'checkpoint-plots',
TEMPLATE_PLOTS = 'template-plots',
Expand All @@ -17,12 +28,11 @@ export const DEFAULT_SECTION_NB_ITEMS_PER_ROW = {
[Section.CUSTOM_PLOTS]: DEFAULT_NB_ITEMS_PER_ROW
}

// Height is undefined by default because it is calculated by ratio of the width it'll fill (calculated by the webview)
export const DEFAULT_HEIGHT = {
[Section.CHECKPOINT_PLOTS]: undefined,
[Section.TEMPLATE_PLOTS]: undefined,
[Section.COMPARISON_TABLE]: undefined,
[Section.CUSTOM_PLOTS]: undefined
[Section.CHECKPOINT_PLOTS]: DEFAULT_PLOT_HEIGHT,
[Section.TEMPLATE_PLOTS]: DEFAULT_PLOT_HEIGHT,
[Section.COMPARISON_TABLE]: DEFAULT_PLOT_HEIGHT,
[Section.CUSTOM_PLOTS]: DEFAULT_PLOT_HEIGHT
}

export const DEFAULT_SECTION_COLLAPSED = {
Expand Down Expand Up @@ -60,7 +70,7 @@ export type Revision = {
export interface PlotsComparisonData {
plots: ComparisonPlots
nbItemsPerRow: number
height: number | undefined
height: PlotHeight
revisions: Revision[]
}

Expand Down Expand Up @@ -93,7 +103,7 @@ export type CustomPlotData = {
export type CustomPlotsData = {
plots: CustomPlotData[]
nbItemsPerRow: number
height: number | undefined
height: PlotHeight
}

export type CheckpointPlotData = CheckpointPlot & { title: string }
Expand All @@ -102,7 +112,7 @@ export type CheckpointPlotsData = {
plots: CheckpointPlotData[]
colors: ColorScale
nbItemsPerRow: number
height: number | undefined
height: PlotHeight
selectedMetrics?: string[]
}

Expand Down Expand Up @@ -151,7 +161,7 @@ export type TemplatePlotSection = {
export interface TemplatePlotsData {
plots: TemplatePlotSection[]
nbItemsPerRow: number
height: number | undefined
height: PlotHeight
}

export type ComparisonPlot = {
Expand Down
Loading