fix(Radar): Radar chart normalisation#33559
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
|
@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@geido Ephemeral environment spinning up at http://34.213.64.49:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
Outdated
Show resolved
Hide resolved
| const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); | ||
| const numberFormatter = getNumberFormatter(numberFormat); | ||
| const denormalizedSeriesValues: SeriesNormalizedMap = {}; | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Unnecessary Boolean constructor ▹ view | ✅ Fix detected | |
| Inefficient Value Lookup Pattern ▹ view | ✅ Fix detected | |
| Inconsistent Series Name Fallback ▹ view | ✅ Fix detected | |
| Complex Value Formatting Logic ▹ view | ✅ Fix detected | |
| Unsafe Tooltip Formatter Typing ▹ view | ✅ Fix detected | |
| Normalization Zero Division Edge Case ▹ view | 🧠 Incorrect |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Radar/types.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx
Outdated
Show resolved
Hide resolved
| const { name = '', value } = params; | ||
| const formattedValue = numberFormatter(value as number); | ||
| const formattedValue = numberFormatter( | ||
| isNormalised | ||
| ? (getDenormalisedSeriesValue(name, String(value)) as number) | ||
| : (value as number), | ||
| ); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if ( | ||
| Object.prototype.hasOwnProperty.call( | ||
| denormalizedSeriesValues, | ||
| seriesName, | ||
| ) && | ||
| Object.prototype.hasOwnProperty.call( | ||
| denormalizedSeriesValues[seriesName], | ||
| normalisedValue, | ||
| ) | ||
| ) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| 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.
This comment was marked as resolved.
Sorry, something went wrong.
| }, | ||
| ]; | ||
|
|
||
| const NormalisedTooltipFormaterr = function (params: any) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // Fallback: return the normalised value itself as a number | ||
| return Number(normalisedValue); | ||
| }; |
There was a problem hiding this comment.
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
|
@Antonio-RiveroMartnez Ephemeral environment spinning up at http://54.214.198.178:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
@kgabryje Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@kgabryje Ephemeral environment spinning up at http://54.202.38.228:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
I didn't follow the discussion too closely here, but shouldn't we keep the control for changing the scales? Also, we should make sure that the existing charts are not affected, and the best way to do it would be to either set the control to false by default, or if we need it to be enabled by default, then we should create a migration for existing Radar charts. |
superset-frontend/plugins/plugin-chart-echarts/src/Radar/NormalizedTooltip.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
Outdated
Show resolved
Hide resolved
Hi @kgabryje , as per discussion with @betodealmeida @geido , Since the radar chart was broken , we have agreed not to keep backwards compatibility & no toggle or control will be available hence the pr got changed from feature to fix since this is a fix for radar chart Also for the normalisation naming , it was a visiblity issue , that whether if we show the end user normalisation or not.... But on an implementation level , we are normalising the variables on scale of 0 to 1 with respect to max metric value hence renaming wont be required |
|
Thanks for explanation @amaannawab923. Approving but it would be great if @betodealmeida could give it a final pass |
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts
Show resolved
Hide resolved
msyavuz
left a comment
There was a problem hiding this comment.
Thanks for this, maybe we should have more fitting examples for different charts. Some example data do not fit the chart use cases at all.
|
Please give it a final pass @betodealmeida , Thanks |
|
@msyavuz Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@msyavuz Ephemeral environment spinning up at http://34.219.31.206:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
Co-authored-by: Amaan Nawab <nelsondrew07@gmail.com>
Co-authored-by: Amaan Nawab <nelsondrew07@gmail.com> (cherry picked from commit bdfb698)
🔧 Summary
This PR introduces a fix for for the Radar Chart in Superset, addressing a critical issue where large discrepancies in metric magnitudes made the chart visually misleading and unusable for comparison.
🐛 The Issue
When plotting multiple metrics with vastly different scales (e.g., 208000, 2590, 9200), the Radar Chart currently uses a faulty calculation mechanism which renders just a Polygon because each value occupies the max on its axis. This causes small values to appear indistinguishable from large ones—rendering the chart ineffective for visual comparison.
🎥 Before (no normalization):
As it can be seen in above screenshot 797.73 & 2.43k appear visually equivalent, even though they arent & the end user is not able to differentiate between them through the radar chart
🧠 Root Cause
Radar Charts plot all metric values using a common 0-to-max scale. This approach ignores the fact that some metrics may span much smaller ranges. As a result:
A metric like 2.5k appears to stretch just as far as 208k
There’s no visual cue for the viewer to recognize magnitude differences
✅ The Fix
This PR introduces:
A new normalisation function in Radar chart where each metric’s values are scaled between 0 and 1
This results in equal visual weight per metric, regardless of absolute value
Safe denormalization logic for tooltips and labels
Tooltips and labels show original values, not normalized ones
Denormalization map tracks each series’ normalized-to-original value mapping
🧪 How It Works
The Normalize function activates per-series normalization during data transformation.
Original values are stored in a SeriesNormalizedMap structure.
Custom formatters use this map to denormalize values when displaying them in tooltips or labels.
🖼️ UX Enhancements
Tooltip and label values now reflect true, raw numbers even when normalized values are plotted.
Improves clarity when comparing metrics with different scales.
🎥 After (with normalization):
Now 797.73 appears visually appropriate in comparison to 2.43k
Metrics now appear proportionally within their own scale, improving accuracy and readability.
🧰 Developer Notes
Introduced new tooltip formatter and label formatter handling
Implemented safe getDenormalisedSeriesValue() accessor
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION