Time shift difference #5177

betodealmeida · 2018-06-11T23:18:45Z

I added a new control to allow comparing comparing the main time series with time shifts. In addition to showing the time shift as individual lines, it's now possible to compute absolute and relative differences.

Current behavior, will be the default option in the selector ("individual lines"):

Absolute difference:

Relative difference, computed as (main - time_shift) / time_shift (for a "1 week" shift" this means that 100 means the value today is double the value last week, and -50 means that the value today is half of what it was last week):

mistercrunch · 2018-06-11T23:25:53Z

I think this is effectively the same as a superior Period Ratio isn't it? If so we should db migrate to translate the slices.

I know one difference in the backend is that Period Ratio runs 2 queries and merges them. So I could look at the past 28 days and compare with last year's same period. I'm not sure if Time Shift can do that or it would query/show more than 28 days (which may be ok). What do you think?

codecov-io · 2018-06-11T23:36:22Z

Codecov Report

Merging #5177 into master will decrease coverage by 15.57%.
The diff coverage is 39.53%.

@@             Coverage Diff             @@
##           master    #5177       +/-   ##
===========================================
- Coverage   76.78%   61.21%   -15.58%     
===========================================
  Files          44      373      +329     
  Lines        8944    23709    +14765     
  Branches        0     2750     +2750     
===========================================
+ Hits         6868    14513     +7645     
- Misses       2076     9181     +7105     
- Partials        0       15       +15

Impacted Files	Coverage Δ
superset/assets/src/explore/controls.jsx	`46.96% <0%> (ø)`
superset/assets/src/visualizations/nvd3_vis.js	`8.14% <0%> (ø)`
superset/assets/src/explore/visTypes.jsx	`57.14% <100%> (ø)`
superset/viz.py	`77.34% <27.27%> (-0.51%)`	⬇️
superset/assets/src/SqlLab/actions.js	`71.25% <50%> (ø)`
superset/assets/src/explore/store.js	`23.07% <50%> (ø)`
.../src/explore/components/ControlPanelsContainer.jsx	`88.31% <72.72%> (ø)`
superset/db_engine_specs.py	`54.07% <0%> (-0.23%)`	⬇️
...et/assets/src/dashboard/reducers/dashboardState.js	`94.59% <0%> (ø)`
superset/assets/src/components/ModalTrigger.jsx	`97.05% <0%> (ø)`
... and 328 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c445ef8...f0a72de. Read the comment docs.

betodealmeida · 2018-06-12T19:55:04Z

I think this is effectively the same as a superior Period Ratio isn't it? If so we should db migrate to translate the slices.

I'm not familiar with the period ratio, and it doesn't seem to map 1:1 to the comparison types. Should I create a migration script only for those cases that can be mapped?

I know one difference in the backend is that Period Ratio runs 2 queries and merges them. So I could look at the past 28 days and compare with last year's same period. I'm not sure if Time Shift can do that or it would query/show more than 28 days (which may be ok). What do you think?

How I would that with the period ratio? For time shift you would select the last 28 days and then use "1 year" as the time shift.

vylc · 2018-06-12T22:38:47Z

Some updates to the AA section to improve usability

betodealmeida · 2018-06-14T21:24:35Z

…me_shift_difference

mistercrunch · 2018-06-19T04:49:10Z

superset/assets/src/explore/controls.jsx

@@ -1562,24 +1562,6 @@ export const controls = {
    description: t('Compute the contribution to the total'),
  },

-  num_period_compare: {


If deprecating num_period_compare and period_ratio_type, do we need a db migration for existing charts that have been saved using that feature?

mistercrunch · 2018-06-19T05:05:16Z

LGTM apart from the db migration needed comment.

…me_shift_difference

betodealmeida · 2018-07-07T03:05:25Z

@mistercrunch, I've added a migration script. Here's a dashboard that was saved before the migration:

Note that:

granularity is month;
num_period_compare is 24 (months);
period_ratio_type is "growth" (ie, new/old - 1).

After the migration:

Now:

time_compare is "2 years" (though it doesn't show in the dropdown since it's free form);
comparison_type is "percentage" (same as "growth", just renamed).

…me_shift_difference

mistercrunch · 2018-07-16T19:17:42Z

LGTM

williaster · 2018-07-17T19:13:20Z

@betodealmeida this broke my local build, the db migration fails. I think you need to check if granularity is defined.

timifasubaa · 2018-07-17T19:27:48Z

Same here

williaster · 2018-07-24T00:14:14Z

superset/assets/package.json

@@ -68,6 +68,7 @@
    "geojson-extent": "^0.3.2",
    "geolib": "^2.0.24",
    "immutable": "^3.8.2",
+    "is-react": "^1.1.1",


@betodealmeida @mistercrunch was it necessary to add this dependency for functionality that is already provided by React natively?

from the source:
React.isValidElement(typeElement)

FYI this added react 16 as a direct dependency (not a peer dependency), which means we have 2 versions of react floating around. it'd be great to get rid of this.

Let me take a look.

Fixed in #5478.

* Allow different comparisons when using time shift * Remove test code * Remove lead/trail NaN and improve code * Add headers/subheader * Update yarn * Migration script * Fix migration * Small fixes * Trigger tests * Fix lint * Fix javascript

* Allow different comparisons when using time shift * Remove test code * Remove lead/trail NaN and improve code * Add headers/subheader * Update yarn * Migration script * Fix migration * Small fixes * Trigger tests * Fix lint * Fix javascript (cherry picked from commit 7b4e6c7)

michellethomas · 2018-08-04T00:19:55Z

superset/viz.py

+                    self.to_series(
+                        diff, classed='time-shift-{}'.format(i), title_suffix=label))
+
+        return sorted(chart_data, key=lambda x: tuple(x['key']))


@betodealmeida we've been seeing issues with the stacked line chart where the lines are not sorted correctly. The stacked chart has sort_series set to true, and that sorting is done before this gets called, so I think this sorting is causing the issue. Is it necessary to sort here?

Taking a look at this.

IIRC this is needed in order to color the groups in different timeshifts with the same color (eg, make sure "boys" is red in the main series as well as all offsets).

* Allow different comparisons when using time shift * Remove test code * Remove lead/trail NaN and improve code * Add headers/subheader * Update yarn * Migration script * Fix migration * Small fixes * Trigger tests * Fix lint * Fix javascript

betodealmeida added 3 commits June 11, 2018 12:44

Allow different comparisons when using time shift

64d3444

Remove test code

3daba3c

Remove lead/trail NaN and improve code

a4c0dd4

Add headers/subheader

88901e7

betodealmeida added 2 commits June 15, 2018 16:47

Rebase

2b06f59

Merge branch 'master' of github.com:apache/incubator-superset into ti…

88f406f

…me_shift_difference

mistercrunch reviewed Jun 19, 2018

View reviewed changes

betodealmeida added 5 commits July 5, 2018 13:44

Merge branch 'master' of github.com:apache/incubator-superset into ti…

21d1a7a

…me_shift_difference

Update yarn

81ba5b3

Migration script

e713816

Fix migration

196785a

Small fixes

b3cbad5

betodealmeida added 4 commits July 9, 2018 10:09

Merge branch 'master' of github.com:apache/incubator-superset into ti…

7d3919f

…me_shift_difference

Trigger tests

c303b46

Merge

39abc63

Fix lint

6cf7cd2

betodealmeida added 2 commits July 16, 2018 15:09

Delete package-lock.json

249958f

Fix javascript

f0a72de

mistercrunch merged commit 7b4e6c7 into apache:master Jul 16, 2018

graceguo-supercat mentioned this pull request Jul 23, 2018

fix migration 3dda56f1c #5468

Merged

williaster reviewed Jul 24, 2018

View reviewed changes

graceguo-supercat mentioned this pull request Jul 24, 2018

[cherry-pick] migrate all dashboards airbnb/superset-fork#66

Merged

michellethomas reviewed Aug 4, 2018

View reviewed changes

michellethomas mentioned this pull request Nov 9, 2018

Fixing sort issue with area chart and adding tests #6358

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time shift difference #5177

Time shift difference #5177

betodealmeida commented Jun 11, 2018

mistercrunch commented Jun 11, 2018

codecov-io commented Jun 11, 2018 •

edited

Loading

betodealmeida commented Jun 12, 2018

vylc commented Jun 12, 2018

betodealmeida commented Jun 14, 2018

mistercrunch Jun 19, 2018

mistercrunch commented Jun 19, 2018

betodealmeida commented Jul 7, 2018

mistercrunch commented Jul 16, 2018

williaster commented Jul 17, 2018 •

edited

Loading

timifasubaa commented Jul 17, 2018

williaster Jul 24, 2018

williaster Jul 24, 2018

betodealmeida Jul 24, 2018

betodealmeida Jul 24, 2018

michellethomas Aug 4, 2018

betodealmeida Aug 9, 2018

betodealmeida Aug 9, 2018

Time shift difference #5177

Time shift difference #5177

Conversation

betodealmeida commented Jun 11, 2018

mistercrunch commented Jun 11, 2018

codecov-io commented Jun 11, 2018 • edited Loading

Codecov Report

betodealmeida commented Jun 12, 2018

vylc commented Jun 12, 2018

betodealmeida commented Jun 14, 2018

Choose a reason for hiding this comment

mistercrunch commented Jun 19, 2018

betodealmeida commented Jul 7, 2018

mistercrunch commented Jul 16, 2018

williaster commented Jul 17, 2018 • edited Loading

timifasubaa commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 11, 2018 •

edited

Loading

williaster commented Jul 17, 2018 •

edited

Loading