[explore] new visualization that adds a paired t-test table to linechart #3006
Conversation
| }; | ||
| } | ||
|
|
||
| statsPairedTTest(aa, bb) { | ||
| // this function calculates the paired t-test values between two arraysß |
aayushshah15
force-pushed
the
paired-ttest-viz
branch
2 times, most recently
from
f2d2109
to
87e994d
Compare
| @@ -302,7 +303,7 @@ function nvd3Vis(slice, payload) { | |||
| chart.height(height); | |||
| slice.container.css('height', height + 'px'); | |||
|
|
|||
| if ((vizType === 'line' || vizType === 'area') && fd.rich_tooltip) { | |||
| if (((vizType === 'line' || vizType === 'area') || vizType === 'line_ttest') && fd.rich_tooltip) { | |||
There was a problem hiding this comment.
At this point let's to with:
(['line', 'area', 'line_ttest'].indexOf(vizType) >= 0)
| }; | ||
| } | ||
|
|
||
| statsPairedTTest(aa, bb) { |
There was a problem hiding this comment.
This does not belong in ExploreViewContainer, I think it is specific to nvd3.jsx at this point and should probably be defined there.
This PR shouldn't touch ExploreViewContainer at all.
| @@ -15,6 +15,51 @@ | |||
| margin-right: 0px; | |||
| } | |||
|
|
|||
| .table-container { | |||
There was a problem hiding this comment.
should be in nvd3.css and the css should be namespaced to only affect what is under .nvd3
| .then((results) => { | ||
| const data = results.data; | ||
| this.setState({ data: data }); | ||
| this.computePairedTTest(0); |
There was a problem hiding this comment.
0 seems arbitrary. It seems like you should be able to define what you want the control to be (?)
There was a problem hiding this comment.
0 is just the default index. The user can select any stream to be the control by clicking on the stream's row in the table.
.gitignore
Outdated
| @@ -10,6 +10,7 @@ _static | |||
| _images | |||
| _modules | |||
| superset/bin/supersetc | |||
| superset/assets/backendSync.json | |||
|
Please fix the linter errors :) |
aayushshah15
changed the title
aayushshah15
force-pushed
the
paired-ttest-viz
branch
2 times, most recently
from
256b27b
to
7eb9140
Compare
|
I made the changes that were requested. The paired t-test logic is now all in the PairedTTestTableContainer. The ExploreViewContainer is mostly untouched except some logic to calculate the chart height based on the height of the paired t-test table. |
aayushshah15
force-pushed
the
paired-ttest-viz
branch
from
c339b78
to
b2b4bb2
Compare
|
Tested locally, and verified that it works. |
aayushshah15
force-pushed
the
paired-ttest-viz
branch
from
a975522
to
659f834
Compare
|
Coverage increased (+0.01%) to 69.265% when pulling 528f2d7c2bff06143284917a86db91f8a9e32b50 on aayushshah15:paired-ttest-viz into a141695 on apache:master. |
1 similar comment
|
Coverage increased (+0.01%) to 69.265% when pulling 528f2d7c2bff06143284917a86db91f8a9e32b50 on aayushshah15:paired-ttest-viz into a141695 on apache:master. |
aayushshah15
force-pushed
the
paired-ttest-viz
branch
from
528f2d7
to
7bfa4c1
Compare
1 similar comment
| </Panel> | ||
| </div> | ||
| {this.props.viz_type === 'line_ttest' && | ||
| <PairedTTestTableContainer />} |
There was a problem hiding this comment.
New visualizations should not live here. You probably want to call ReactDom.render from within nvd3.jsx. No need to touch this file at all.
| standalone: PropTypes.bool.isRequired, | ||
| triggerQuery: PropTypes.bool.isRequired, | ||
| queryResponse: PropTypes.object, |
There was a problem hiding this comment.
You shouldn't have to touch this file at all. queryResponse will be available in your visualization context in nvd3.jsx
| import dist from 'distributions'; | ||
|
|
||
| import { connect } from 'react-redux'; | ||
| import { Table } from 'react-bootstrap'; |
There was a problem hiding this comment.
We've been using reactable, it offers good features out of the box (sorting, filtering)
| @@ -940,6 +945,8 @@ export const visTypes = { | |||
| }, | |||
| }, | |||
| }; | |||
| visTypes.line_ttest.controlPanelSections = visTypes.line.controlPanelSections; | |||
There was a problem hiding this comment.
Seems like this would be easy to miss. I'd prefer defining const lineVis = {...} at the top and then
line: lineVis,
line_ttest: Object.assign({}, lineVis, {'label': '...'}),
| background-color: white; | ||
| } | ||
|
|
||
| h4.nvd3-paired-ttest-table .message-outline { |
There was a problem hiding this comment.
Would rather you use bootstrap CSS classes in your JSX as opposed to custom css
|
@mistercrunch @aayushshah15 finished his internship, and we are continuing the development of this feature in a new pull request: #3473 |
|
Notice: this issue has been closed because it has been inactive for 219 days. Feel free to comment and request for this issue to be reopened. |
A table located below the line chart displays the paired t-test and lift values of all time-series’ with respect to a set “control” (which, by default, is the first time-series represented in the table). The user can click on any of the rows to set that particular row as the “control” and all the values will be recomputed and the table, re-rendered. The table will be disabled when more than one metric is selected.
The paired t-test values that are > 0.05 are colored red to indicate that they are not statistically significant.
The table is also recomputed and re-rendered after each query triggered by the user. The chart and the table are placed inside a scrollable div and the height of the line chart is reduced dynamically based on the height of the table, up to a set minimum of 500 pixels.
Supervised by @joshwalters