fix: Reordering echart props to fix confidence interval in Mixed Charts

[geotab-data-platform]

fix(echarts): confidence interval does not display correctly for mixed time series charts with negative values

SUMMARY

The root cause of this problem lies within the order of the arguments sent to the echarts library. When working with time-series data, Echarts enforces specific constraints on how the x-axis bounds are configured. These constraints include:
In other words, the Echarts Library accepts input in this specific format and will malfunction otherwise. Superset does not create the Echarts modal with the options in this order.

• The xaxis.type option must be explicitly set to 'time'.
• The upper and lower bound values must be equal to the lower bound.
• The upper bound must be defined before the lower bound in the options object.

Currently, the code in Superset that generates the Echarts configuration does not adhere to this required format, leading to the observed malfunction. This pull request rectifies this issue by reordering the upper and lower bound arguments before they are passed to the Echarts library.

What is currently being sent

What should actually be sent

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

create a mixed chart on the explore menu using confidence intervals where the names__yhat_lower bound is negative at least once.

Verify that confidence Intervals behave as Intended.

names__yhat: trend/point estimate
names__yhat_lower: lower confidence level
names__yhat_upper: upper confidence level

ADDITIONAL INFORMATION

Fixes #30554

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[villebro]

@geotab-data-platform I don't see this PR changing enforcing or updating the xaxis.type = 'time' issue, despite it being changed in the "after" example. Is this not changed here?

[vedantprajapati17]

xaxis type defaults to "value" if not reordered and there are negative bound values. When its ordered properly, it uses "time".

[geotab-data-platform]

@villebro any updates?

[villebro]

@vedantprajapati17 I am unable to properly review this PR due to the following:

• I'm unable to reproduce the issue without dedicating considerable time for this. For instance, the fact that the before/after option payloads aren't diffable (one is JS, the other is JSON) doesn't help.
• I still don't understand how xaxis.type changes as a consequence of this PR, despite there being no such change in the PR.

Also, we would need a minimal unit test to validate that reorderForecastSeries works as expected. This both helps understand how the function works, and will help refactoring and protect against regressions going forward.

[villebro]

@geotab-data-platform please let me know if you need help with this. I'd love to pull this fix in, but in the absence of more clarity on how this fix works it will be difficult to merge this.

[geotab-data-platform]

Hi @villebro,
Please see attached the current behaviour vs after the fix.

Settings Used:

Chart Type: Mixed Chart
yhat = 1
yhat_lower = -10
yhat_upper = 5
query b = 10

Current Behaviour

As you can see, the confidence interval ranges between (0,15) instead of (-10,5).

After the fix

Setting the same values we get the following response with correct confidence intervals (-10,5).

When you do not reorder the props and send them to echarts as a json to generate the chart, the echarts package will default to an x axis that uses value and not time. Additionally, the props must be ordered in the that specific order as the behaviour will not be as expected otherwise and confidence bands will not be correct.

[vedantprajapati17]

Hi @villebro, any updates?

[geotab-data-platform]

Any updates on this MR?

[rusackas]

Running CI... 🤞

[vedantprajapati]

Running CI... 🤞

Seems like it passed? Failled to pull some images for 2 pipelines @rusackas

[rusackas]

Sorry for the delay. I think we're still just agonizing over how this works and whether it's safe to merge, or might just cause some other edge case to appear. If it's possible to add some form of tests (unit tests or end to end tests) that would help immensely.

[vedantprajapati]

Sorry for the delay. I think we're still just agonizing over how this works and whether it's safe to merge, or might just cause some other edge case to appear. If it's possible to add some form of tests (unit tests or end to end tests) that would help immensely.

It's difficult to validate whether or not a canvas html component looks one way compared to another. Especially when all this function just reorders the series that is being sent. ie no data is removed or added. I can add unit tests validating that the order is changed correctly? @rusackas

[rusackas]

I think the unit test would help... at least it would harden the feature against someone accidentally changing the code in a way that reverts the functionality.

[rusackas]

/testenv up

[github-actions]

@rusackas Processing your ephemeral environment request here.

[vedantprajapati]

tests added