Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(generic-axes): apply contribution before flatten #20077

Merged
merged 1 commit into from May 16, 2022

Conversation

villebro
Copy link
Member

@villebro villebro commented May 16, 2022

SUMMARY

Currently the contribution operation is applied after flattening, despite only being meant to apply to numeric columns. By ensuring that the dataframe is unflattened, the index columns will always be absent from the columns, which ensures that the index is not used in the contribution calculation. Also, in the case of using a numeric index, applying the contribution calculation after flattening would cause the index to leak into the contribution calculation. In addition to the fix, falsy post transformation ops are removed to clean up the request payload (these cause unnecessary cache misses).

There is already a check that should remove any non-numeric columns:

numeric_df = contribution_df.select_dtypes(include=["number", Decimal])

However, for some reason this currently also returns string-columns (object), possibly due to the Decimal type extending object (seems weird, I know, but the only reason I could come up with). However, I didn't want to make any changes to this logic in this PR, as refining it will probably require additional tests to be added + we should potentially bump Pandas to 1.4 before changing this.

AFTER

Now calculating contributions works when GENERIC_CHART_AXES enabled:
image

BEFORE

Previously it would fail:
image

TESTING INSTRUCTIONS

  1. Enable GENERIC_CHART_AXES feature flag
  2. Create a chart with a non-temporal x-axis
  3. Enable contribution mode (either one is ok)

ADDITIONAL INFORMATION

  • 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

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

prophetOperator(formData, baseQueryObject),
],
].filter(op => op),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The filter should apply in a separate PR and apply in all post operators.

@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #20077 (e94f064) into master (4435e53) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #20077   +/-   ##
=======================================
  Coverage   66.37%   66.38%           
=======================================
  Files        1715     1715           
  Lines       64179    64180    +1     
  Branches     6753     6753           
=======================================
+ Hits        42602    42603    +1     
  Misses      19859    19859           
  Partials     1718     1718           
Flag Coverage Δ
javascript 51.33% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../plugin-chart-echarts/src/Timeseries/buildQuery.ts 71.42% <100.00%> (+4.76%) ⬆️

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 4435e53...e94f064. Read the comment docs.

@villebro villebro merged commit d5802f7 into apache:master May 16, 2022
@villebro villebro deleted the villebro/generic-contribution branch May 16, 2022 13:29
villebro added a commit that referenced this pull request May 26, 2022
michael-s-molina pushed a commit that referenced this pull request May 26, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@mistercrunch mistercrunch added 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XS 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants