-
Notifications
You must be signed in to change notification settings - Fork 17.8k
fix(Radar): Radar chart normalisation #33559
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
Changes from 17 commits
7a9f687
1cec095
696d5d9
672c16f
3df058e
dc5820d
dce9fe3
7f51dca
e88e98d
4e92bb6
6d6d237
0e4927a
e0fd65f
b374343
9daf1a6
0c1f934
d0e4da2
11f3ba2
009dec2
11dd197
0cc2183
4cd764d
022ed85
b7d608a
e0959d2
a6a13a6
9c6606e
66707a8
a82c59d
edde6c9
b158611
bf2fb5e
86478bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ import { | |
| EchartsRadarFormData, | ||
| EchartsRadarLabelType, | ||
| RadarChartTransformedProps, | ||
| SeriesNormalizedMap, | ||
| } from './types'; | ||
| import { DEFAULT_LEGEND_FORM_DATA, OpacityEnum } from '../constants'; | ||
| import { | ||
|
|
@@ -51,13 +52,24 @@ export function formatLabel({ | |
| params, | ||
| labelType, | ||
| numberFormatter, | ||
| getDenormalisedSeriesValue, | ||
| isNormalised, | ||
| }: { | ||
| params: CallbackDataParams; | ||
| labelType: EchartsRadarLabelType; | ||
| numberFormatter: NumberFormatter; | ||
| getDenormalisedSeriesValue: ( | ||
| seriesName: string, | ||
| normalisedValue: string, | ||
| ) => number; | ||
| isNormalised: boolean; | ||
| }): string { | ||
| const { name = '', value } = params; | ||
| const formattedValue = numberFormatter(value as number); | ||
| const formattedValue = numberFormatter( | ||
| isNormalised | ||
| ? (getDenormalisedSeriesValue(name, String(value)) as number) | ||
| : (value as number), | ||
| ); | ||
|
Comment on lines
59
to
+75
This comment was marked as resolved.
Sorry, something went wrong. |
||
|
|
||
| switch (labelType) { | ||
| case EchartsRadarLabelType.Value: | ||
|
|
@@ -103,6 +115,7 @@ export default function transformProps( | |
| isCircle, | ||
| columnConfig, | ||
| sliceId, | ||
| isNormalised, | ||
| }: EchartsRadarFormData = { | ||
| ...DEFAULT_LEGEND_FORM_DATA, | ||
| ...DEFAULT_RADAR_FORM_DATA, | ||
|
|
@@ -111,11 +124,36 @@ export default function transformProps( | |
| const { setDataMask = () => {}, onContextMenu } = hooks; | ||
| const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); | ||
| const numberFormatter = getNumberFormatter(numberFormat); | ||
| const denormalizedSeriesValues: SeriesNormalizedMap = {}; | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use an Object to store the denormalised values belonging to each series because after normalising we need to pass the denormalised values to the label formatter & tooltip formatter to keep the chart consistent & preserve original values |
||
| const getDenormalisedSeriesValue = ( | ||
|
amaannawab923 marked this conversation as resolved.
Outdated
|
||
| seriesName: string, | ||
| normalisedValue: string, | ||
| ): number => { | ||
| if ( | ||
| Object.prototype.hasOwnProperty.call( | ||
| denormalizedSeriesValues, | ||
| seriesName, | ||
| ) && | ||
| Object.prototype.hasOwnProperty.call( | ||
| denormalizedSeriesValues[seriesName], | ||
| normalisedValue, | ||
| ) | ||
|
amaannawab923 marked this conversation as resolved.
Outdated
|
||
| ) { | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
| return denormalizedSeriesValues[seriesName][normalisedValue]; | ||
| } | ||
|
|
||
| // Fallback: return the normalised value itself as a number | ||
| return Number(normalisedValue); | ||
| }; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The getDenormalisedSeriesValue is getter function for safely getting the denormalised value , if we dont find the value we fallback to normalised value, this shouldnt happen but we do this to prevent crashes & unexpected errors which can crash the user experience |
||
|
|
||
| const formatter = (params: CallbackDataParams) => | ||
| formatLabel({ | ||
| params, | ||
| numberFormatter, | ||
| labelType, | ||
| getDenormalisedSeriesValue, | ||
| isNormalised, | ||
| }); | ||
|
|
||
| const metricLabels = metrics.map(getMetricLabel); | ||
|
|
@@ -124,7 +162,7 @@ export default function transformProps( | |
| const metricLabelAndMaxValueMap = new Map<string, number>(); | ||
| const metricLabelAndMinValueMap = new Map<string, number>(); | ||
| const columnsLabelMap = new Map<string, string[]>(); | ||
| const transformedData: RadarSeriesDataItemOption[] = []; | ||
| let transformedData: RadarSeriesDataItemOption[] = []; | ||
|
amaannawab923 marked this conversation as resolved.
Outdated
|
||
| data.forEach(datum => { | ||
| const joinedName = extractGroupbyLabel({ | ||
| datum, | ||
|
|
@@ -212,7 +250,40 @@ export default function transformProps( | |
| {}, | ||
| ); | ||
|
|
||
| // Add normalization function | ||
| const normalizeArray = (arr: number[], decimals = 10, seriesName: string) => { | ||
| const max = Math.max(...arr); | ||
| return arr.map(value => { | ||
| const normalisedValue = Number((value / max).toFixed(decimals)); | ||
| denormalizedSeriesValues[seriesName][String(normalisedValue)] = value; | ||
| return normalisedValue; | ||
| }); | ||
| }; | ||
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The normalise function gets max value from each series & consider it as 1.0 and then calculates the normalised value & then goes onto put a reference of normalised -> denormalised value in our denormalised values map |
||
|
|
||
| if (isNormalised) { | ||
| // Normalize the transformed data | ||
| transformedData = transformedData.map(series => { | ||
| if (Array.isArray(series.value)) { | ||
| const seriesName = String(series?.name || ''); | ||
| denormalizedSeriesValues[seriesName] = {}; | ||
|
|
||
| return { | ||
| ...series, | ||
| value: normalizeArray(series.value as number[], 10, seriesName), | ||
| }; | ||
| } | ||
| return series; | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We normalise the values for each series only if we have isNormalised as true |
||
| } | ||
|
|
||
| const indicator = metricLabels.map(metricLabel => { | ||
| if (isNormalised) { | ||
| return { | ||
| name: metricLabel, | ||
| max: 1, | ||
| min: 0, | ||
| }; | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the values are normalised we consider min & max as 0 & 1 respectively & hence we dont need to calculate the rest as they are only in case when isNormalised is not true & some column config is provided |
||
| const maxValueInControl = columnConfig?.[metricLabel]?.radarMetricMaxValue; | ||
| const minValueInControl = columnConfig?.[metricLabel]?.radarMetricMinValue; | ||
|
|
||
|
|
@@ -259,6 +330,39 @@ export default function transformProps( | |
| }, | ||
| ]; | ||
|
|
||
| const NormalisedTooltipFormater = ( | ||
| params: CallbackDataParams & { | ||
| color: string; | ||
| name: string; | ||
| value: number[]; | ||
| }, | ||
| ) => { | ||
| const { color } = params; | ||
| const seriesName = params.name || ''; | ||
| const values = params.value; | ||
|
|
||
| const colorDot = `<span style="display:inline-block;margin-right:5px;border-radius:50%;width:5px;height:5px;background-color:${color}"></span>`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could move NormalisedTooltipFormatter to a separate .tsx file and use JSX instead of strings
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes thanks ... Made a seperate tooltip component
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Echarts expects a string and renderToStaticMarkup causes a performance hit. See #33501. We should probably revert this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| // Start with series name without dot | ||
| let tooltip = `<div style="font-weight:bold;margin-bottom:5px;">${seriesName || 'series0'}</div>`; | ||
|
amaannawab923 marked this conversation as resolved.
Outdated
|
||
|
|
||
| metricLabels.forEach((metric, index) => { | ||
| const normalizedValue = values[index]; | ||
| const originalValue = getDenormalisedSeriesValue( | ||
| seriesName, | ||
| String(normalizedValue), | ||
| ); | ||
| tooltip += ` | ||
| <div style="display:flex;"> | ||
| <div>${colorDot}${metric}:</div> | ||
| <div style="font-weight:bold;margin-left:auto;">${originalValue}</div> | ||
| </div> | ||
| `; | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we need to override the denormalised values with the normalised values in tooltip we create the tooltip from scratch with the tooltip formatter in the format that echarts expect which is html string |
||
|
|
||
| return tooltip; | ||
| }; | ||
|
|
||
| const echartOptions: EChartsCoreOption = { | ||
| grid: { | ||
| ...defaultGrid, | ||
|
|
@@ -267,6 +371,9 @@ export default function transformProps( | |
| ...getDefaultTooltip(refs), | ||
| show: !inContextMenu, | ||
| trigger: 'item', | ||
| ...(isNormalised && { | ||
| formatter: NormalisedTooltipFormater, | ||
| }), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use this normalised tooltip formatter only when normlalised is true |
||
| }, | ||
| legend: { | ||
| ...getLegendProps(legendType, legendOrientation, showLegend, theme), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,7 @@ export type EchartsRadarFormData = QueryFormData & | |
| isCircle: boolean; | ||
| numberFormat: string; | ||
| dateFormat: string; | ||
| isNormalised: boolean; | ||
| }; | ||
|
|
||
| export enum EchartsRadarLabelType { | ||
|
|
@@ -83,3 +84,17 @@ export type RadarChartTransformedProps = | |
| BaseTransformedProps<EchartsRadarFormData> & | ||
| ContextMenuTransformedProps & | ||
| CrossFilterTransformedProps; | ||
|
|
||
| /** | ||
| * Represents a mapping from a normalized value (as string) to an original numeric value. | ||
| */ | ||
| interface NormalizedValueMap { | ||
| [normalized: string]: number; | ||
| } | ||
|
|
||
| /** | ||
| * Represents a collection of series, each containing its own NormalizedValueMap. | ||
| */ | ||
| export interface SeriesNormalizedMap { | ||
| [seriesName: string]: NormalizedValueMap; | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interfaces of Series -> Normalised -> denormalised values |
||
Uh oh!
There was an error while loading. Please reload this page.