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.

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

Jump to bottom

feat: Display better x-axis ticks on charts with time axis [WEB-849] #6051

Merged
merged 3 commits into from
Feb 23, 2023
Merged

feat: Display better x-axis ticks on charts with time axis [WEB-849] #6051

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3318363
fix dynamix axis/scale, move getTimeTickValues for use
mapmeld Feb 22, 2023
f038997
use default time ticks on longer experiments
mapmeld Feb 22, 2023
1ebc2d5
remove xTickValues from props now that it can be calculated internally
mapmeld Feb 23, 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
30 changes: 24 additions & 6 deletions webui/react/src/components/kit/LineChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { tooltipsPlugin } from 'components/UPlot/UPlotChart/tooltipsPlugin2';
import useResize from 'hooks/useResize';
import { glasbeyColor } from 'shared/utils/color';
import { MetricType, Scale } from 'types';
import { getTimeTickValues } from 'utils/chart';

import css from './LineChart.module.scss';

Expand Down Expand Up @@ -124,6 +125,18 @@ export const LineChart: React.FC<Props> = ({
return [xValues, ...yValuesArray];
}, [series, xAxis]);

const xTicksAdjusted: uPlot.Axis.Values | undefined = useMemo(
() =>
xTickValues ??
Copy link
Contributor

Choose a reason for hiding this comment

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

once we remove the feature flag, can we remove xTickValues from the props and just do this internally?

in the meantime xTicksAdjusted seems a little misleading because if the user passes xTickValues we are just leaving them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TrialDetails/Profiles is the one place using xTickValues and yTickValues props, so I probably could just move it here, I didn't know if there was future interest in keeping it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok the last commit removes the xTickValues prop, this only affects the Profiler, example is http://localhost:3000/det/experiments/1788/profiler?f_chart=on

then I renamed xTicksAdjusted as the new xTickValues

we're still accepting yTickValues as a prop from Profiler; I can make that into a separate ticket

(xAxis === XAxisDomain.Time &&
chartData.length > 0 &&
chartData[0].length > 0 &&
chartData[0][chartData[0].length - 1] - chartData[0][0] < 43200) // 12 hours
? getTimeTickValues
: undefined,
[chartData, xAxis, xTickValues],
);

const chartOptions: Options = useMemo(() => {
const plugins: Plugin[] = propPlugins ?? [
tooltipsPlugin({
Expand All @@ -148,7 +161,7 @@ export const LineChart: React.FC<Props> = ({
side: 2,
space: 120,
ticks: { show: false },
values: xTickValues,
values: xTicksAdjusted,
},
{
font: '12px "Objektiv Mk3", Arial, Helvetica, sans-serif',
Expand Down Expand Up @@ -195,7 +208,7 @@ export const LineChart: React.FC<Props> = ({
onPointClick,
onPointFocus,
xLabel,
xTickValues,
xTicksAdjusted,
yLabel,
yTickValues,
height,
Expand Down Expand Up @@ -258,9 +271,14 @@ export interface GroupProps {
* `data` comes from the itemData prop that is passed to FixedSizeGrid.
*/
const VirtualChartRenderer: React.FC<
GridChildComponentProps<{ chartsProps: ChartsProps; columnCount: number }>
GridChildComponentProps<{
chartsProps: ChartsProps;
columnCount: number;
scale: Scale;
xAxis: XAxisDomain;
}>
> = ({ columnIndex, rowIndex, style, data }) => {
const { chartsProps, columnCount } = data;
const { chartsProps, columnCount, scale, xAxis } = data;

const cellIndex = rowIndex * columnCount + columnIndex;

Expand All @@ -270,7 +288,7 @@ const VirtualChartRenderer: React.FC<
return (
<div className={css.chartgridCell} key={`${rowIndex}, ${columnIndex}`} style={style}>
<div className={css.chartgridCellCard}>
<LineChart {...chartProps} scale={Scale.Linear} xAxis={XAxisDomain.Batches} />
<LineChart {...chartProps} scale={scale} xAxis={xAxis} />
</div>
</div>
);
Expand Down Expand Up @@ -310,7 +328,7 @@ export const ChartGrid: React.FC<GroupProps> = React.memo(
columnCount={columnCount}
columnWidth={Math.floor(width / columnCount)}
height={Math.min(height - 40, (chartsProps.length > columnCount ? 2.1 : 1.05) * 480)}
itemData={{ chartsProps, columnCount }}
itemData={{ chartsProps, columnCount, scale, xAxis }}
rowCount={Math.ceil(chartsProps.length / columnCount)}
rowHeight={480}
width={width}>
Expand Down
3 changes: 2 additions & 1 deletion webui/react/src/pages/TrialDetails/Profiles/Charts/SystemMetricChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import { LineChart } from 'components/kit/LineChart';
import { XAxisDomain } from 'components/kit/LineChart/XAxisFilter';
import Section from 'components/Section';
import { SettingsConfig, useSettings } from 'hooks/useSettings';
import { getTimeTickValues } from 'utils/chart';

import { ChartProps } from '../types';
import { MetricType } from '../types';
import { useFetchProfilerMetrics } from '../useFetchProfilerMetrics';
import { useFetchProfilerSeries } from '../useFetchProfilerSeries';
import { getScientificNotationTickValues, getTimeTickValues, getUnitForMetricName } from '../utils';
import { getScientificNotationTickValues, getUnitForMetricName } from '../utils';

import SystemMetricFilter from './SystemMetricChartFilters';

Expand Down
3 changes: 2 additions & 1 deletion webui/react/src/pages/TrialDetails/Profiles/Charts/ThroughputMetricChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ import React from 'react';
import { LineChart } from 'components/kit/LineChart';
import { XAxisDomain } from 'components/kit/LineChart/XAxisFilter';
import Section from 'components/Section';
import { getTimeTickValues } from 'utils/chart';

import { ChartProps } from '../types';
import { MetricType } from '../types';
import { useFetchProfilerMetrics } from '../useFetchProfilerMetrics';
import { getScientificNotationTickValues, getTimeTickValues, getUnitForMetricName } from '../utils';
import { getScientificNotationTickValues, getUnitForMetricName } from '../utils';

const ThroughputMetricChart: React.FC<ChartProps> = ({ trial }) => {
const throughputMetrics = useFetchProfilerMetrics(
Expand Down
3 changes: 2 additions & 1 deletion webui/react/src/pages/TrialDetails/Profiles/Charts/TimingMetricChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ import React from 'react';
import { LineChart } from 'components/kit/LineChart';
import { XAxisDomain } from 'components/kit/LineChart/XAxisFilter';
import Section from 'components/Section';
import { getTimeTickValues } from 'utils/chart';

import { ChartProps } from '../types';
import { MetricType } from '../types';
import { useFetchProfilerMetrics } from '../useFetchProfilerMetrics';
import { getScientificNotationTickValues, getTimeTickValues, getUnitForMetricName } from '../utils';
import { getScientificNotationTickValues, getUnitForMetricName } from '../utils';

export const TimingMetricChart: React.FC<ChartProps> = ({ trial }) => {
const timingMetrics = useFetchProfilerMetrics(trial.id, trial.state, MetricType.Timing);
Expand Down
5 changes: 0 additions & 5 deletions webui/react/src/pages/TrialDetails/Profiles/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import dayjs from 'dayjs';
import uPlot from 'uplot';

// key should be lowercase to match the metric name
Expand All @@ -21,10 +20,6 @@ export const getUnitForMetricName = (metricName: string): string => {
: metricName;
};

export const getTimeTickValues: uPlot.Axis['values'] = (_self, rawValue) => {
return rawValue.map((val) => dayjs.unix(val).format('hh:mm:ss.SSS').slice(0, -2));
};

export const getScientificNotationTickValues: uPlot.Axis['values'] = (_self, rawValue) => {
return rawValue.map((val) => {
if (val === 0) return val;
Expand Down
7 changes: 7 additions & 0 deletions webui/react/src/utils/chart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import dayjs from 'dayjs';
import uPlot from 'uplot';

import { Theme } from 'shared/themes';
import { Primitive, Range } from 'shared/types';
import { ColorScale } from 'shared/utils/color';
Expand Down Expand Up @@ -75,3 +78,7 @@ export const normalizeRange = (values: number[], range: Range<number>): number[]
export function distance(x0: number, y0: number, x1: number, y1: number): number {
return Math.sqrt((x1 - x0) ** 2 + (y1 - y0) ** 2);
}

export const getTimeTickValues: uPlot.Axis.Values = (_self, rawValue) => {
return rawValue.map((val) => dayjs.unix(val).format('hh:mm:ss.SSS').slice(0, -2));
};