From 4f17a3d9f186aaa9dff8d49775e696cde51c256d Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 19 Dec 2022 12:03:54 -0600 Subject: [PATCH 1/3] Refactor `getDataFromColumnPath` logic * fix bug with long integers * simplify code and reduce duplication --- .../src/experiments/model/quickPick.test.ts | 2 +- extension/src/experiments/model/quickPicks.ts | 18 +++---- extension/src/experiments/model/tree.ts | 18 +++---- extension/src/experiments/model/util.ts | 35 ++++++++----- extension/src/plots/model/util.ts | 30 ++++-------- extension/src/plots/webview/contract.ts | 3 +- .../src/test/fixtures/plotsDiff/index.ts | 39 +++++---------- .../test/suite/experiments/model/tree.test.ts | 49 ++++++------------- .../experiments/util/buildDynamicColumns.tsx | 2 +- .../plots/components/ribbon/RibbonBlock.tsx | 33 ++++++++----- .../numberFormatting.ts => util/number.ts} | 3 +- 11 files changed, 94 insertions(+), 138 deletions(-) rename webview/src/{experiments/util/numberFormatting.ts => util/number.ts} (71%) diff --git a/extension/src/experiments/model/quickPick.test.ts b/extension/src/experiments/model/quickPick.test.ts index 744e90a662..c948d0333e 100644 --- a/extension/src/experiments/model/quickPick.test.ts +++ b/extension/src/experiments/model/quickPick.test.ts @@ -147,7 +147,7 @@ describe('pickExperiments', () => { description: '[exp-456]', detail: `Created:${formatDate( mockedExperiments[1].Created as string - )}, split:2.2000e+9, data/data.xml:22a1a29`, + )}, split:2200043556, data/data.xml:22a1a29`, label: '456fsf4', value: mockedExperiments[1] }, diff --git a/extension/src/experiments/model/quickPicks.ts b/extension/src/experiments/model/quickPicks.ts index e658115a7f..ffb658d9f3 100644 --- a/extension/src/experiments/model/quickPicks.ts +++ b/extension/src/experiments/model/quickPicks.ts @@ -2,7 +2,7 @@ import { QuickPickItemKind } from 'vscode' import omit from 'lodash.omit' import { ExperimentWithCheckpoints } from '.' import { MAX_SELECTED_EXPERIMENTS } from './status' -import { getDataFromColumnPath } from './util' +import { getDataFromColumnPaths } from './util' import { definedAndNonEmpty } from '../../util/array' import { QuickPickItemWithValue, @@ -25,17 +25,11 @@ const getSeparator = (experiment: Experiment) => ({ }) const getItem = (experiment: Experiment, firstThreeColumnOrder: string[]) => ({ - detail: firstThreeColumnOrder - .map(path => { - const { splitUpPath, value } = getDataFromColumnPath(experiment, path) - const truncatedKey = truncateFromLeft( - splitUpPath[splitUpPath.length - 1], - 15 - ) - - return value === null ? '' : `${truncatedKey}:${value}` - }) - .filter(Boolean) + detail: getDataFromColumnPaths(experiment, firstThreeColumnOrder) + .map( + ({ splitUpPath, truncatedValue: value }) => + `${truncateFromLeft(splitUpPath[splitUpPath.length - 1], 15)}:${value}` + ) .join(', '), label: experiment.label, value: omit(experiment, 'checkpoints') diff --git a/extension/src/experiments/model/tree.ts b/extension/src/experiments/model/tree.ts index d3a6f4cee3..50d2e12c23 100644 --- a/extension/src/experiments/model/tree.ts +++ b/extension/src/experiments/model/tree.ts @@ -12,7 +12,7 @@ import { ExperimentType } from '.' import { ExperimentAugmented } from './filterBy/collect' import { collectDeletable, ExperimentItem } from './collect' import { MAX_SELECTED_EXPERIMENTS } from './status' -import { getDataFromColumnPath } from './util' +import { getDataFromColumnPaths } from './util' import { WorkspaceExperiments } from '../workspace' import { sendViewOpenedTelemetryEvent } from '../../telemetry' import { EventName } from '../../telemetry/constants' @@ -347,18 +347,12 @@ export class ExperimentsTree } private getTooltipTable(experiment: Experiment, firstThreeColumns: string[]) { - const data = firstThreeColumns - .map(path => { - const { value, splitUpPath } = getDataFromColumnPath(experiment, path) - const [type] = splitUpPath - const truncatedKey = truncateFromLeft( - path.slice(type.length + 1) || path, - 30 - ) - return value === null ? '' : `| ${truncatedKey} | ${value} |\n` - }) + const data = getDataFromColumnPaths(experiment, firstThreeColumns) + .map( + ({ truncatedValue: value, columnPath }) => + `| ${truncateFromLeft(columnPath, 30)} | ${value} |\n` + ) .join('') - return data === '' ? undefined : getMarkdownString(`|||\n|:--|--|\n${data}`) } diff --git a/extension/src/experiments/model/util.ts b/extension/src/experiments/model/util.ts index 7acd4441fa..7976f76d87 100644 --- a/extension/src/experiments/model/util.ts +++ b/extension/src/experiments/model/util.ts @@ -8,8 +8,10 @@ type Value = undefined | null | [] | string | number const isDate = (value: Value): boolean => !!(typeof value === 'string' && Date.parse(value)) -const isLongNumber = (value: Value): boolean => - typeof value === 'number' && value.toString().length > 7 +const isLongFloatNumber = (value: Value): boolean => + typeof value === 'number' && + !Number.isInteger(value as number) && + value.toString().length > 7 const getStringifiedValue = (value: Value): string => { if (Number.isNaN(value)) { @@ -28,29 +30,38 @@ const getStringifiedValue = (value: Value): string => { return '-' } - if (isLongNumber(value)) { + if (isLongFloatNumber(value)) { return (value as number).toPrecision(5) } return String(value) } -export const getDataFromColumnPath = ( +const getDataFromColumnPath = ( experiment: Experiment, columnPath: string -): { fullValue: string; splitUpPath: string[]; value: string | null } => { +): { + type: string + value: string | number + columnPath: string + splitUpPath: string[] + truncatedValue: string +} => { const splitUpPath = splitColumnPath(columnPath) const collectedVal = get(experiment, splitUpPath) const value = collectedVal?.value || collectedVal + const [type] = splitUpPath return { - fullValue: isLongNumber(value) - ? value.toString() - : getStringifiedValue(value), + columnPath: columnPath.slice(type.length + 1) || columnPath, splitUpPath, - value: - columnPath === 'Created' && value === 'undefined' - ? null - : getStringifiedValue(value) + truncatedValue: getStringifiedValue(value), + type, + value: isLongFloatNumber(value) ? value : getStringifiedValue(value) } } + +export const getDataFromColumnPaths = ( + experiment: Experiment, + columnPaths: string[] +) => columnPaths.map(path => getDataFromColumnPath(experiment, path)) diff --git a/extension/src/plots/model/util.ts b/extension/src/plots/model/util.ts index 67dc7d59bc..f4b5f81e96 100644 --- a/extension/src/plots/model/util.ts +++ b/extension/src/plots/model/util.ts @@ -1,27 +1,15 @@ -import { getDataFromColumnPath } from '../../experiments/model/util' +import { getDataFromColumnPaths } from '../../experiments/model/util' import { Experiment } from '../../experiments/webview/contract' import { RevisionFirstThreeColumns } from '../webview/contract' export const getRevisionFirstThreeColumns = ( firstThreeColumns: string[], experiment: Experiment -) => { - const columns: RevisionFirstThreeColumns = [] - for (const path of firstThreeColumns) { - const { value, splitUpPath, fullValue } = getDataFromColumnPath( - experiment, - path - ) - const type = splitUpPath[0] - - if (value) { - columns.push({ - fullValue, - path: path.slice(type.length + 1) || path, - type, - value - }) - } - } - return columns -} +): RevisionFirstThreeColumns => + getDataFromColumnPaths(experiment, firstThreeColumns).map( + ({ columnPath: path, value, type }) => ({ + path, + type, + value + }) + ) diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index 7b0fafd5fd..2a6fe69181 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -37,8 +37,7 @@ export type ComparisonPlots = { export type RevisionFirstThreeColumns = Array<{ path: string - value: string - fullValue: string + value: string | number type: string }> diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index 328f8b0381..6fd9df4239 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -542,21 +542,18 @@ export const getRevisions = (): Revision[] => { firstThreeColumns: [ { type: 'Created', - fullValue: '-', path: 'Created', value: '-' }, { type: ColumnType.METRICS, - fullValue: '1.9293040037155151', path: 'summary.json:loss', - value: '1.9293' + value: '1.9293040037155151' }, { type: ColumnType.METRICS, - fullValue: '0.4668000042438507', path: 'summary.json:accuracy', - value: '0.46680' + value: '0.4668000042438507' } ], group: undefined @@ -566,21 +563,18 @@ export const getRevisions = (): Revision[] => { firstThreeColumns: [ { type: 'Created', - fullValue: formatDate(getMain().Created as string), path: 'Created', value: formatDate(getMain().Created as string) }, { - type: ColumnType.METRICS, - fullValue: '2.048856019973755', path: 'summary.json:loss', - value: '2.0489' + type: ColumnType.METRICS, + value: '2.048856019973755' }, { - type: ColumnType.METRICS, - fullValue: '0.3484833240509033', path: 'summary.json:accuracy', - value: '0.34848' + type: ColumnType.METRICS, + value: '0.3484833240509033' } ], id: 'main', @@ -593,21 +587,18 @@ export const getRevisions = (): Revision[] => { firstThreeColumns: [ { type: 'Created', - fullValue: findAndFormatCreated('exp-e7a67'), path: 'Created', value: findAndFormatCreated('exp-e7a67') }, { type: ColumnType.METRICS, - fullValue: '2.0205044746398926', path: 'summary.json:loss', - value: '2.0205' + value: '2.0205044746398926' }, { type: ColumnType.METRICS, - fullValue: '0.3724166750907898', path: 'summary.json:accuracy', - value: '0.37242' + value: '0.3724166750907898' } ], id: 'exp-e7a67', @@ -619,22 +610,19 @@ export const getRevisions = (): Revision[] => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('test-branch'), type: 'Created', path: 'Created', value: findAndFormatCreated('test-branch') }, { - fullValue: '1.9293040037155151', type: ColumnType.METRICS, path: 'summary.json:loss', - value: '1.9293' + value: '1.9293040037155151' }, { - fullValue: '0.4668000042438507', type: ColumnType.METRICS, path: 'summary.json:accuracy', - value: '0.46680' + value: '0.4668000042438507' } ], id: 'test-branch', @@ -646,22 +634,19 @@ export const getRevisions = (): Revision[] => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('exp-83425'), type: 'Created', path: 'Created', value: findAndFormatCreated('exp-83425') }, { - fullValue: '1.775016188621521', type: ColumnType.METRICS, path: 'summary.json:loss', - value: '1.7750' + value: '1.775016188621521' }, { - fullValue: '0.5926499962806702', type: ColumnType.METRICS, path: 'summary.json:accuracy', - value: '0.59265' + value: '0.5926499962806702' } ], id: 'exp-83425', diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index ace25eba70..185f199420 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -410,22 +410,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: '-', path: 'Created', type: 'Created', value: '-' }, { - fullValue: '1.9293040037155151', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '1.9293' + value: 1.9293040037155151 }, { - fullValue: '0.4668000042438507', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.46680' + value: 0.4668000042438507 } ], group: undefined, @@ -437,22 +434,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('exp-e7a67'), path: 'Created', type: 'Created', value: findAndFormatCreated('exp-e7a67') }, { - fullValue: '2.0205044746398926', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '2.0205' + value: 2.0205044746398926 }, { - fullValue: '0.3724166750907898', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.37242' + value: 0.3724166750907898 } ], group: '[exp-e7a67]', @@ -464,22 +458,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('exp-e7a67'), path: 'Created', type: 'Created', value: findAndFormatCreated('exp-e7a67') }, { - fullValue: '2.0205044746398926', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '2.0205' + value: 2.0205044746398926 }, { - fullValue: '0.3724166750907898', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.37242' + value: 0.3724166750907898 } ], group: '[exp-e7a67]', @@ -491,22 +482,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('test-branch'), path: 'Created', type: 'Created', value: findAndFormatCreated('test-branch') }, { - fullValue: '1.9293040037155151', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '1.9293' + value: 1.9293040037155151 }, { - fullValue: '0.4668000042438507', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.46680' + value: 0.4668000042438507 } ], group: '[test-branch]', @@ -518,22 +506,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('exp-e7a67'), path: 'Created', type: 'Created', value: findAndFormatCreated('exp-e7a67') }, { - fullValue: '2.020392894744873', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '2.0204' + value: 2.020392894744873 }, { - fullValue: '0.3723166584968567', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.37232' + value: 0.3723166584968567 } ], group: '[exp-e7a67]', @@ -545,22 +530,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('test-branch'), path: 'Created', type: 'Created', value: findAndFormatCreated('test-branch') }, { - fullValue: '1.9293040037155151', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '1.9293' + value: 1.9293040037155151 }, { - fullValue: '0.4668000042438507', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.46680' + value: 0.4668000042438507 } ], group: '[test-branch]', @@ -572,22 +554,19 @@ suite('Experiments Tree Test Suite', () => { fetched: true, firstThreeColumns: [ { - fullValue: findAndFormatCreated('test-branch'), path: 'Created', type: 'Created', value: findAndFormatCreated('test-branch') }, { - fullValue: '1.9882521629333496', path: 'summary.json:loss', type: ColumnType.METRICS, - value: '1.9883' + value: 1.9882521629333496 }, { - fullValue: '0.4083833396434784', path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.40838' + value: 0.4083833396434784 } ], group: '[test-branch]', diff --git a/webview/src/experiments/util/buildDynamicColumns.tsx b/webview/src/experiments/util/buildDynamicColumns.tsx index 019e0ffb2d..cda15e6a0a 100644 --- a/webview/src/experiments/util/buildDynamicColumns.tsx +++ b/webview/src/experiments/util/buildDynamicColumns.tsx @@ -14,7 +14,6 @@ import { ValueWithChanges } from 'dvc/src/experiments/webview/contract' import { Value } from 'dvc/src/cli/dvc/contract' -import { formatFloat } from './numberFormatting' import Tooltip, { NORMAL_TOOLTIP_DELAY } from '../../shared/components/tooltip/Tooltip' @@ -22,6 +21,7 @@ import styles from '../components/table/styles.module.scss' import { CopyButton } from '../../shared/components/copyButton/CopyButton' import { OverflowHoverTooltip } from '../components/overflowHoverTooltip/OverflowHoverTooltip' import { ErrorTooltip } from '../components/table/Errors' +import { formatFloat } from '../../util/number' export type CellValue = Value | ValueWithChanges diff --git a/webview/src/plots/components/ribbon/RibbonBlock.tsx b/webview/src/plots/components/ribbon/RibbonBlock.tsx index 88f6eabaeb..74edf60eed 100644 --- a/webview/src/plots/components/ribbon/RibbonBlock.tsx +++ b/webview/src/plots/components/ribbon/RibbonBlock.tsx @@ -8,6 +8,7 @@ import { Icon } from '../../../shared/components/Icon' import Tooltip from '../../../shared/components/tooltip/Tooltip' import { CopyButton } from '../../../shared/components/copyButton/CopyButton' import { Close } from '../../../shared/components/icons' +import { formatFloat } from '../../../util/number' interface RibbonBlockProps { revision: Revision @@ -28,19 +29,25 @@ export const RibbonBlock: React.FC = ({ const tooltipContent = ( - {revision.firstThreeColumns.map(({ path, value, type, fullValue }) => ( - - - - - ))} + {revision.firstThreeColumns.map(({ path, value, type }) => { + const isFloat = typeof value === 'number' && !Number.isInteger(value) + return ( + + + + + ) + })}
- {truncate(path, 45, 'left')} - - {value} - {value === '-' || ( - - )} -
+ {truncate(path, 45, 'left')} + + {isFloat ? formatFloat(value) : value} + {value === '-' || ( + + )} +
) diff --git a/webview/src/experiments/util/numberFormatting.ts b/webview/src/util/number.ts similarity index 71% rename from webview/src/experiments/util/numberFormatting.ts rename to webview/src/util/number.ts index f230d89b94..3dd4951b92 100644 --- a/webview/src/experiments/util/numberFormatting.ts +++ b/webview/src/util/number.ts @@ -1,6 +1,5 @@ -const defaultPrecision = 5 // for when we can't calculate real precision yet - export const formatFloat = (value: number): string => { + const defaultPrecision = 5 // for when we can't calculate real precision yet const automatic = value.toString() if (automatic.length > 7) { return value.toPrecision(defaultPrecision) From 98887588cc5f0408d1ac7d7b61d4f2250f8f52fe Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 19 Dec 2022 12:14:01 -0600 Subject: [PATCH 2/3] Fix vscode tests --- .../src/test/fixtures/plotsDiff/index.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index 6fd9df4239..14076a426d 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -548,12 +548,12 @@ export const getRevisions = (): Revision[] => { { type: ColumnType.METRICS, path: 'summary.json:loss', - value: '1.9293040037155151' + value: 1.9293040037155151 }, { type: ColumnType.METRICS, path: 'summary.json:accuracy', - value: '0.4668000042438507' + value: 0.4668000042438507 } ], group: undefined @@ -569,12 +569,12 @@ export const getRevisions = (): Revision[] => { { path: 'summary.json:loss', type: ColumnType.METRICS, - value: '2.048856019973755' + value: 2.048856019973755 }, { path: 'summary.json:accuracy', type: ColumnType.METRICS, - value: '0.3484833240509033' + value: 0.3484833240509033 } ], id: 'main', @@ -593,12 +593,12 @@ export const getRevisions = (): Revision[] => { { type: ColumnType.METRICS, path: 'summary.json:loss', - value: '2.0205044746398926' + value: 2.0205044746398926 }, { type: ColumnType.METRICS, path: 'summary.json:accuracy', - value: '0.3724166750907898' + value: 0.3724166750907898 } ], id: 'exp-e7a67', @@ -617,12 +617,12 @@ export const getRevisions = (): Revision[] => { { type: ColumnType.METRICS, path: 'summary.json:loss', - value: '1.9293040037155151' + value: 1.9293040037155151 }, { type: ColumnType.METRICS, path: 'summary.json:accuracy', - value: '0.4668000042438507' + value: 0.4668000042438507 } ], id: 'test-branch', @@ -641,12 +641,12 @@ export const getRevisions = (): Revision[] => { { type: ColumnType.METRICS, path: 'summary.json:loss', - value: '1.775016188621521' + value: 1.775016188621521 }, { type: ColumnType.METRICS, path: 'summary.json:accuracy', - value: '0.5926499962806702' + value: 0.5926499962806702 } ], id: 'exp-83425', From 0841455ad4ec29bb137abdb762d992dcd6fd0459 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 20 Dec 2022 09:26:04 -0600 Subject: [PATCH 3/3] Add back long number truncation --- extension/src/experiments/model/quickPick.test.ts | 2 +- extension/src/experiments/model/util.ts | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/extension/src/experiments/model/quickPick.test.ts b/extension/src/experiments/model/quickPick.test.ts index c948d0333e..744e90a662 100644 --- a/extension/src/experiments/model/quickPick.test.ts +++ b/extension/src/experiments/model/quickPick.test.ts @@ -147,7 +147,7 @@ describe('pickExperiments', () => { description: '[exp-456]', detail: `Created:${formatDate( mockedExperiments[1].Created as string - )}, split:2200043556, data/data.xml:22a1a29`, + )}, split:2.2000e+9, data/data.xml:22a1a29`, label: '456fsf4', value: mockedExperiments[1] }, diff --git a/extension/src/experiments/model/util.ts b/extension/src/experiments/model/util.ts index 7976f76d87..c5048115de 100644 --- a/extension/src/experiments/model/util.ts +++ b/extension/src/experiments/model/util.ts @@ -8,10 +8,8 @@ type Value = undefined | null | [] | string | number const isDate = (value: Value): boolean => !!(typeof value === 'string' && Date.parse(value)) -const isLongFloatNumber = (value: Value): boolean => - typeof value === 'number' && - !Number.isInteger(value as number) && - value.toString().length > 7 +const isLongNumber = (value: Value): boolean => + typeof value === 'number' && value.toString().length > 7 const getStringifiedValue = (value: Value): string => { if (Number.isNaN(value)) { @@ -30,7 +28,7 @@ const getStringifiedValue = (value: Value): string => { return '-' } - if (isLongFloatNumber(value)) { + if (isLongNumber(value)) { return (value as number).toPrecision(5) } @@ -57,7 +55,7 @@ const getDataFromColumnPath = ( splitUpPath, truncatedValue: getStringifiedValue(value), type, - value: isLongFloatNumber(value) ? value : getStringifiedValue(value) + value: isLongNumber(value) ? value : getStringifiedValue(value) } }