Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
20 changes: 6 additions & 14 deletions extension/src/plots/model/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { PlotsModel } from '.'
import {
DefaultNumberOfItemsPerRow,
DEFAULT_SECTION_COLLAPSED,
DEFAULT_SECTION_NB_ITEMS_PER_ROW,
PlotNumberOfItemsPerRow,
Section
} from '../webview/contract'
import { buildMockMemento } from '../../test/util'
Expand Down Expand Up @@ -69,33 +69,25 @@ describe('plotsModel', () => {

it('should change the plotSize when calling setPlotSize', () => {
expect(model.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)).toStrictEqual(
PlotNumberOfItemsPerRow.TWO
DefaultNumberOfItemsPerRow
)

model.setNbItemsPerRow(
Section.CHECKPOINT_PLOTS,
PlotNumberOfItemsPerRow.ONE
)
model.setNbItemsPerRow(Section.CHECKPOINT_PLOTS, 1)

expect(model.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)).toStrictEqual(
PlotNumberOfItemsPerRow.ONE
)
expect(model.getNbItemsPerRow(Section.CHECKPOINT_PLOTS)).toStrictEqual(1)
})

it('should update the persisted plot size when calling setPlotSize', () => {
const mementoUpdateSpy = jest.spyOn(memento, 'update')

model.setNbItemsPerRow(
Section.CHECKPOINT_PLOTS,
PlotNumberOfItemsPerRow.TWO
)
model.setNbItemsPerRow(Section.CHECKPOINT_PLOTS, 2)

expect(mementoUpdateSpy).toHaveBeenCalledTimes(1)
expect(mementoUpdateSpy).toHaveBeenCalledWith(
PersistenceKey.PLOT_NB_ITEMS_PER_ROW + exampleDvcRoot,
{
...DEFAULT_SECTION_NB_ITEMS_PER_ROW,
[Section.CHECKPOINT_PLOTS]: PlotNumberOfItemsPerRow.TWO
[Section.CHECKPOINT_PLOTS]: 2
}
)
})
Expand Down
16 changes: 4 additions & 12 deletions extension/src/plots/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ import {
Section,
SectionCollapsed,
CustomPlotData,
PlotNumberOfItemsPerRow,
DEFAULT_HEIGHT
DEFAULT_HEIGHT,
DefaultNumberOfItemsPerRow
} from '../webview/contract'
import {
ExperimentsOutput,
Expand Down Expand Up @@ -408,18 +408,10 @@ export class PlotsModel extends ModelWithPersistence {
}

public getNbItemsPerRow(section: Section) {
if (
this.nbItemsPerRow[section] &&
[
PlotNumberOfItemsPerRow.ONE,
PlotNumberOfItemsPerRow.TWO,
PlotNumberOfItemsPerRow.THREE,
PlotNumberOfItemsPerRow.FOUR
].includes(this.nbItemsPerRow[section])
) {
if (this.nbItemsPerRow[section]) {
return this.nbItemsPerRow[section]
}
return PlotNumberOfItemsPerRow.TWO
return DefaultNumberOfItemsPerRow
}

public setHeight(section: Section, height: number | undefined) {
Expand Down
34 changes: 15 additions & 19 deletions extension/src/plots/vega/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import scatterTemplate from '../../test/fixtures/plotsDiff/templates/scatter'
import smoothTemplate from '../../test/fixtures/plotsDiff/templates/smooth'
import multiSourceTemplate from '../../test/fixtures/plotsDiff/templates/multiSource'
import { copyOriginalColors } from '../../experiments/model/status/colors'
import { PlotNumberOfItemsPerRow } from '../webview/contract'
import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
import { DefaultNumberOfItemsPerRow } from '../webview/contract'

describe('isMultiViewPlot', () => {
it('should recognize the confusion matrix template as a multi view plot', () => {
Expand Down Expand Up @@ -86,7 +86,7 @@ describe('extendVegaSpec', () => {
it('should not add encoding if no color scale is provided', () => {
const extendedSpec = extendVegaSpec(
linearTemplate,
PlotNumberOfItemsPerRow.TWO
DefaultNumberOfItemsPerRow
)
expect(extendedSpec.encoding).toBeUndefined()
})
Expand All @@ -98,7 +98,7 @@ describe('extendVegaSpec', () => {
}
const extendedSpec = extendVegaSpec(
linearTemplate,
PlotNumberOfItemsPerRow.TWO,
DefaultNumberOfItemsPerRow,
{
color: colorScale
}
Expand Down Expand Up @@ -143,7 +143,7 @@ describe('extendVegaSpec', () => {

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

const truncatedTitle = '…-many-many-characters-at-least-seventy-characters'
const truncatedHorizontalTitle =
Expand All @@ -169,7 +169,7 @@ describe('extendVegaSpec', () => {

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

const truncatedTitle = '…-many-many-characters-at-least-seventy-characters'
const truncatedHorizontalTitle =
Expand All @@ -195,7 +195,7 @@ describe('extendVegaSpec', () => {

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

const truncatedTitle = '…s-at-least-seventy-characters'
const truncatedHorizontalTitle = '…at-least-seventy-characters-x'
Expand Down Expand Up @@ -225,7 +225,7 @@ describe('extendVegaSpec', () => {
text: repeatedTitle
})

const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE)
const updatedSpec = extendVegaSpec(spec, 3)

const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890'

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

const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE)
const updatedSpec = extendVegaSpec(spec, 3)

const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890'

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

const updatedSpec = extendVegaSpec(spec, PlotNumberOfItemsPerRow.THREE)
const updatedSpec = extendVegaSpec(spec, 3)

const truncatedTitle = '…ghijklmnopqrstuvwyz1234567890'

Expand All @@ -276,17 +276,13 @@ 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,
PlotNumberOfItemsPerRow.ONE,
{
color: { domain: [], range: [] },
shape: {
field: 'field',
scale: { domain: [], range: [] }
}
const updatedSpec = extendVegaSpec(multiSourceTemplate, 1, {
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
13 changes: 3 additions & 10 deletions extension/src/plots/vega/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
} from 'vega-lite/build/src/spec/repeat'
import { TopLevelUnitSpec } from 'vega-lite/build/src/spec/unit'
import isEqual from 'lodash.isequal'
import { ColorScale, PlotNumberOfItemsPerRow } from '../webview/contract'
import { ColorScale } from '../webview/contract'
import { ShapeEncoding, StrokeDashEncoding } from '../multiSource/constants'
import { Color } from '../../experiments/model/status/colors'

Expand Down Expand Up @@ -219,13 +219,6 @@ const truncateTitleAsArrayOrString = (title: Text, size: number) => {
return truncateTitleString(title as unknown as string, size)
}

const TitleLimit = {
[PlotNumberOfItemsPerRow.ONE]: 50,
[PlotNumberOfItemsPerRow.TWO]: 50,
[PlotNumberOfItemsPerRow.THREE]: 30,
[PlotNumberOfItemsPerRow.FOUR]: 30
}

const truncateTitlePart = (
title: Title,
key: 'text' | 'subtitle',
Expand Down Expand Up @@ -260,7 +253,7 @@ const truncateTitle = (
}

export const truncateVerticalTitle = (title: Text | Title, size: number) =>
truncateTitle(title, TitleLimit[size] * 0.75)
truncateTitle(title, (size > 2 ? 30 : 50) * 0.75)

const isEndValue = (valueType: string) =>
['string', 'number', 'boolean'].includes(valueType)
Expand Down Expand Up @@ -288,7 +281,7 @@ export const truncateTitles = (
const title = value as unknown as Title
specCopy[key] = vertical
? truncateVerticalTitle(title, size)
: truncateTitle(title, TitleLimit[size])
: truncateTitle(title, size > 2 ? 30 : 50)
Comment thread
stephr0y marked this conversation as resolved.
Outdated
} else if (isEndValue(valueType)) {
specCopy[key] = value
} else if (Array.isArray(value)) {
Expand Down
18 changes: 5 additions & 13 deletions extension/src/plots/webview/contract.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,7 @@
import { VisualizationSpec } from 'react-vega'
import { Color } from '../../experiments/model/status/colors'

// It is easier to keep the numerical order than the alphabetical one
/* eslint-disable sort-keys-fix/sort-keys-fix */
export const PlotNumberOfItemsPerRow = {
ONE: 1,
TWO: 2,
THREE: 3,
FOUR: 4
}
/* eslint-enable sort-keys-fix/sort-keys-fix */
export const DefaultNumberOfItemsPerRow = 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Q] Should this be calculated based on the screen size?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure myself, just asking the question.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[N] Should use screaming snake case. e.g NUM_OF_COMMITS_TO_SHOW

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[N] Should use screaming snake case. e.g NUM_OF_COMMITS_TO_SHOW

I got confused and was wondering about this because of other variables not using this format. Will change that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Q] Should this be calculated based on the screen size?

Good question. I could try this as a follow up. Currently the default vale is set in the extension, so I'd have to reset everything to undefined to then calculate it in the webview. I'd probably select the middle value by default.


export enum Section {
CHECKPOINT_PLOTS = 'checkpoint-plots',
Expand All @@ -19,10 +11,10 @@ export enum Section {
}

export const DEFAULT_SECTION_NB_ITEMS_PER_ROW = {
[Section.CHECKPOINT_PLOTS]: PlotNumberOfItemsPerRow.TWO,
[Section.TEMPLATE_PLOTS]: PlotNumberOfItemsPerRow.TWO,
[Section.COMPARISON_TABLE]: PlotNumberOfItemsPerRow.TWO,
[Section.CUSTOM_PLOTS]: PlotNumberOfItemsPerRow.TWO
[Section.CHECKPOINT_PLOTS]: DefaultNumberOfItemsPerRow,
[Section.TEMPLATE_PLOTS]: DefaultNumberOfItemsPerRow,
[Section.COMPARISON_TABLE]: DefaultNumberOfItemsPerRow,
[Section.CUSTOM_PLOTS]: DefaultNumberOfItemsPerRow
}

// Height is undefined by default because it is calculated by ratio of the width it'll fill (calculated by the webview)
Expand Down
4 changes: 2 additions & 2 deletions extension/src/test/fixtures/expShow/base/checkpointPlots.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { copyOriginalColors } from '../../../../experiments/model/status/colors'
import {
CheckpointPlotsData,
PlotNumberOfItemsPerRow
DefaultNumberOfItemsPerRow
} from '../../../../plots/webview/contract'

const colors = copyOriginalColors()
Expand Down Expand Up @@ -91,7 +91,7 @@ const data: CheckpointPlotsData = {
'summary.json:val_loss',
'summary.json:val_accuracy'
],
nbItemsPerRow: PlotNumberOfItemsPerRow.TWO,
nbItemsPerRow: DefaultNumberOfItemsPerRow,
height: undefined
}

Expand Down
4 changes: 2 additions & 2 deletions extension/src/test/fixtures/expShow/base/customPlots.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {
CustomPlotsData,
PlotNumberOfItemsPerRow
DefaultNumberOfItemsPerRow
} from '../../../../plots/webview/contract'

const data: CustomPlotsData = {
Expand Down Expand Up @@ -50,7 +50,7 @@ const data: CustomPlotsData = {
]
}
],
nbItemsPerRow: PlotNumberOfItemsPerRow.TWO,
nbItemsPerRow: DefaultNumberOfItemsPerRow,
height: undefined
}

Expand Down
14 changes: 7 additions & 7 deletions extension/src/test/fixtures/plotsDiff/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import {
TemplatePlotGroup,
TemplatePlotsData,
TemplatePlots,
PlotNumberOfItemsPerRow,
Revision,
PlotsComparisonData
PlotsComparisonData,
DefaultNumberOfItemsPerRow
} from '../../../plots/webview/contract'
import { join } from '../../util/path'
import { copyOriginalColors } from '../../../experiments/model/status/colors'
Expand Down Expand Up @@ -499,7 +499,7 @@ const extendedSpecs = (plotsOutput: TemplatePlots): TemplatePlotSection[] => {
) || []
}
} as TopLevelSpec,
PlotNumberOfItemsPerRow.TWO,
DefaultNumberOfItemsPerRow,
{
color: {
domain: expectedRevisions,
Expand Down Expand Up @@ -659,14 +659,14 @@ export const getRevisions = (): Revision[] => {

export const getMinimalWebviewMessage = () => ({
plots: extendedSpecs(basicVega),
nbItemsPerRow: PlotNumberOfItemsPerRow.TWO,
nbItemsPerRow: DefaultNumberOfItemsPerRow,
height: undefined,
revisions: getRevisions()
})

export const getTemplateWebviewMessage = (): TemplatePlotsData => ({
plots: extendedSpecs({ ...basicVega, ...require('./vega').default }),
nbItemsPerRow: PlotNumberOfItemsPerRow.TWO,
nbItemsPerRow: DefaultNumberOfItemsPerRow,
height: undefined
})

Expand All @@ -676,7 +676,7 @@ export const getManyTemplatePlotsWebviewMessage = (
plots: extendedSpecs({
...multipleVega(length)
}),
nbItemsPerRow: PlotNumberOfItemsPerRow.TWO,
nbItemsPerRow: DefaultNumberOfItemsPerRow,
height: undefined
})

Expand All @@ -703,7 +703,7 @@ export const getComparisonWebviewMessage = (
return {
revisions: getRevisions(),
plots: plotAcc,
nbItemsPerRow: PlotNumberOfItemsPerRow.TWO,
nbItemsPerRow: DefaultNumberOfItemsPerRow,
height: undefined
}
}
4 changes: 2 additions & 2 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ import { DvcExecutor } from '../../../cli/dvc/executor'
import { shortenForLabel } from '../../../util/string'
import { GitExecutor } from '../../../cli/git/executor'
import { WorkspacePlots } from '../../../plots/workspace'
import { PlotNumberOfItemsPerRow } from '../../../plots/webview/contract'
import {
RegisteredCliCommands,
RegisteredCommands
Expand All @@ -82,6 +81,7 @@ import * as ProcessExecution from '../../../process/execution'
import { DvcReader } from '../../../cli/dvc/reader'
import { Connect } from '../../../connect'
import { DvcViewer } from '../../../cli/dvc/viewer'
import { DefaultNumberOfItemsPerRow } from '../../../plots/webview/contract'

const { openFileInEditor } = FileSystem

Expand Down Expand Up @@ -339,7 +339,7 @@ suite('Experiments Test Suite', () => {
).returns(undefined)

const mockColumnId = 'params:params.yaml:lr'
const mockWidth = PlotNumberOfItemsPerRow.TWO
const mockWidth = DefaultNumberOfItemsPerRow

mockMessageReceived.fire({
payload: { id: mockColumnId, width: mockWidth },
Expand Down
Loading