You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/02/02 10:06:57 UTC
[GitHub] [superset] villebro opened a new pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
villebro opened a new pull request #18252:
URL: https://github.com/apache/superset/pull/18252
### SUMMARY
This fixes a bug in the ECharts timeseries chart, where using a metric with a verbose name caused the forecast and observations to be on separate series (one with the verbose name, and the other with the raw name). A test is also added that ensures verbose names are correctly handled going forward. In addition, all references to "prophet" are replaced with "forecast" in preparation for decoupling the forecasting functionality from Prophet.
### AFTER
Now the metric with a verbose name displays correctly in both the chart and the tooltip:
The forecast section is also split up into one control per line:
### BEFORE
Previously, the predictions would show up as different series with their non-verbose name, both on the chart and the tooltip
Before, the forecast section headers wrapping caused the controls to be misaligned:
### TESTING INSTRUCTIONS
1. Create a v2 Line chart
2. Add the built-in `COUNT(*)` metric as your primary metric
3. Enable the forecast
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #18252:
URL: https://github.com/apache/superset/pull/18252#issuecomment-1027827385
> The fix doesn't seem to be working well with `GENERIC_CHART_AXES` ff disabled - Y axis is scaled to timestamp epoch values
>
> 
Great catch! It turns out `datasource.verboseMap` always includes an entry `{ __timestamp: 'Time' }` which threw the util function off when it was made more generic. I added added a check for it + added a test for making sure the alias stays unchanged (this logic will be removed once we remove the legacy non-generic timeseries functionality).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a change in pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #18252:
URL: https://github.com/apache/superset/pull/18252#discussion_r797450642
##########
File path: superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
##########
@@ -126,7 +126,8 @@ export default function transformProps(
yAxisTitlePosition,
}: EchartsTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData };
const colorScale = CategoricalColorNamespace.getScale(colorScheme as string);
- const rebasedData = rebaseTimeseriesDatum(data, verboseMap);
+ const rebasedData = rebaseForecastDatum(data, verboseMap);
+ console.log('!!', rebasedData, data, verboseMap);
Review comment:
🙊
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro edited a comment on pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on pull request #18252:
URL: https://github.com/apache/superset/pull/18252#issuecomment-1027827385
> The fix doesn't seem to be working well with `GENERIC_CHART_AXES` ff disabled - Y axis is scaled to timestamp epoch values
Great catch! It turns out `datasource.verboseMap` always includes an entry `{ __timestamp: 'Time' }` which threw the util function off when it was made more generic. I added added a check for it + added a test for making sure the alias stays unchanged (this logic will be removed once we remove the legacy non-generic timeseries functionality).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18252:
URL: https://github.com/apache/superset/pull/18252#issuecomment-1027820069
The fix doesn't seem to be working well with `GENERIC_CHART_AXES` ff disabled - Y axis is scaled to timestamp epoch values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #18252:
URL: https://github.com/apache/superset/pull/18252
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #18252:
URL: https://github.com/apache/superset/pull/18252#issuecomment-1027833167
Retested, works perfectly!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #18252:
URL: https://github.com/apache/superset/pull/18252#discussion_r797467290
##########
File path: superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
##########
@@ -126,7 +126,8 @@ export default function transformProps(
yAxisTitlePosition,
}: EchartsTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData };
const colorScale = CategoricalColorNamespace.getScale(colorScheme as string);
- const rebasedData = rebaseTimeseriesDatum(data, verboseMap);
+ const rebasedData = rebaseForecastDatum(data, verboseMap);
+ console.log('!!', rebasedData, data, verboseMap);
Review comment:
nothing to see here! 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje edited a comment on pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #18252:
URL: https://github.com/apache/superset/pull/18252#issuecomment-1027833167
Retested, works perfectly with and without `GENERIC_CHART_AXES`!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #18252: fix(plugin-chart-echarts): fix forecasts on verbose metrics
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18252:
URL: https://github.com/apache/superset/pull/18252#issuecomment-1027857637
# [Codecov](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18252](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2a4dde5) into [master](https://codecov.io/gh/apache/superset/commit/8c376548e3cf6464e5710e80120f82227174fcdc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8c37654) will **increase** coverage by `0.00%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18252 +/- ##
=======================================
Coverage 66.32% 66.32%
=======================================
Files 1592 1592
Lines 62569 62569
Branches 6295 6295
=======================================
+ Hits 41500 41501 +1
Misses 19416 19416
+ Partials 1653 1652 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.38% <50.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...i-chart-controls/src/sections/forecastInterval.tsx](https://codecov.io/gh/apache/superset/pull/18252/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NlY3Rpb25zL2ZvcmVjYXN0SW50ZXJ2YWwudHN4) | `100.00% <ø> (ø)` | |
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/18252/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ø)` | |
| [...lugin-chart-echarts/src/Timeseries/transformers.ts](https://codecov.io/gh/apache/superset/pull/18252/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1lcnMudHM=) | `50.83% <ø> (ø)` | |
| [...frontend/plugins/plugin-chart-echarts/src/types.ts](https://codecov.io/gh/apache/superset/pull/18252/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvdHlwZXMudHM=) | `100.00% <ø> (ø)` | |
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/18252/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `58.06% <14.28%> (ø)` | |
| [...plugins/plugin-chart-echarts/src/utils/forecast.ts](https://codecov.io/gh/apache/superset/pull/18252/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvdXRpbHMvZm9yZWNhc3QudHM=) | `94.82% <93.33%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8c37654...2a4dde5](https://codecov.io/gh/apache/superset/pull/18252?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org