feat: add rolling window support to 'Big Number with Trendline' viz #9107
Conversation
mistercrunch
force-pushed
the
rolling_big_number
branch
3 times, most recently
from
5110b5b to
af51356
Compare
|
Comments have been addressed |
Codecov Report
@@ Coverage Diff @@
## master #9107 +/- ##
=========================================
- Coverage 58.92% 58.9% -0.03%
=========================================
Files 373 373
Lines 12016 12026 +10
Branches 2948 2953 +5
=========================================
+ Hits 7081 7084 +3
- Misses 4756 4763 +7
Partials 179 179
Continue to review full report at Codecov.
|
|
@willbarrett can you take another look? |
mistercrunch
force-pushed
the
rolling_big_number
branch
from
af51356 to
0e2f471
Compare
| @@ -97,31 +97,32 @@ def load_world_bank_health_n_pop( | |||
| db.session.commit() | |||
| tbl.fetch_metadata() | |||
|
|
|||
| metric = "sum__SP_POP_TOTL" | |||
| metrics = ["sum__SP_POP_TOTL"] | |||
There was a problem hiding this comment.
This is more a general comment relating to the current structure of examples, not specifically this PR. But given the fact that metrics is already defined above (line 80), it might be a good idea to disambiguate here. For example default_metrics, total_population_metrics or similar.
Another option, which I personally would prefer, would be to remove the legacy metrics above, and replace both metric and metrics here with adhoc metrics. Something like
total_population_metric = {
...
}and later on where a metrics list is expected, just passing a [total_population_metric] to highlight that it's a single value list.
| secondary_metric = { | ||
| "aggregate": "SUM", | ||
| "column": { | ||
| "column_name": "SP_RUR_TOTL", | ||
| "optionName": "_col_SP_RUR_TOTL", | ||
| "type": "DOUBLE", | ||
| }, | ||
| "expressionType": "SIMPLE", | ||
| "hasCustomLabel": True, | ||
| "label": "Rural Population", | ||
| } |
There was a problem hiding this comment.
For readability later on in the code, calling this rural_population_metric or similar would be preferable.
| def apply_rolling(self, df): | ||
| fd = self.form_data | ||
| rolling_type = fd.get("rolling_type") | ||
| rolling_periods = int(fd.get("rolling_periods") or 0) | ||
| min_periods = int(fd.get("min_periods") or 0) | ||
|
|
||
| if rolling_type in ("mean", "std", "sum") and rolling_periods: | ||
| kwargs = dict(window=rolling_periods, min_periods=min_periods) | ||
| if rolling_type == "mean": | ||
| df = df.rolling(**kwargs).mean() | ||
| elif rolling_type == "std": | ||
| df = df.rolling(**kwargs).std() | ||
| elif rolling_type == "sum": | ||
| df = df.rolling(**kwargs).sum() | ||
| elif rolling_type == "cumsum": | ||
| df = df.cumsum() | ||
| if min_periods: | ||
| df = df[min_periods:] | ||
| return df |
superset/examples/birth_names.py
Outdated
| ), | ||
| ), | ||
| Slice( | ||
| slice_name="Genders", | ||
| viz_type="pie", | ||
| datasource_type="table", | ||
| datasource_id=tbl.id, | ||
| params=get_slice_json(defaults, viz_type="pie", groupby=["gender"]), | ||
| params=get_slice_json( | ||
| defaults, viz_type="pie", groupby=["gender"], metrics=metrics |
There was a problem hiding this comment.
During testing I noticed that the Gender chart wasn't rendering correctly:
As the pie chart wants the singular metric, you need to change metrics=metrics to metric=metric here.
There was a problem hiding this comment.
good catch, thanks for deep testing this
|
@villebro thank you for the review, addressed your comment! |
…e' viz (apache#9107)" This reverts commit c04d616.
mistercrunch
added
🏷️ bot
CATEGORY
SUMMARY
I was working on a simple dashboard and found myself needing to add support for rolling windows to the 'Big Number with Trendline' viz.
apply_rollingBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Added a unit test
REVIEWERS