-
Notifications
You must be signed in to change notification settings - Fork 14k
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
[SIP-5] Refactor Paired t-test #5762
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5762 +/- ##
==========================================
- Coverage 63.7% 63.68% -0.02%
==========================================
Files 366 367 +1
Lines 23143 23150 +7
Branches 2599 2597 -2
==========================================
Hits 14744 14744
- Misses 8384 8391 +7
Partials 15 15
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, had a couple comments on more specific shapes.
@@ -1,5 +1,5 @@ | |||
.paired_ttest .scrollbar-container { | |||
overflow: scroll; | |||
overflow: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hide all the scroll bars! 🙌
const propTypes = { | ||
className: PropTypes.string, | ||
metrics: PropTypes.arrayOf(PropTypes.string).isRequired, | ||
groups: PropTypes.array.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this one also PropTypes.arrayOf(PropTypes.string).isRequired
?
import './paired_ttest.css'; | ||
const propTypes = { | ||
metric: PropTypes.string.isRequired, | ||
groups: PropTypes.array.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this also PropTypes.arrayOf(PropTypes.string).isRequired
?
const propTypes = { | ||
metric: PropTypes.string.isRequired, | ||
groups: PropTypes.array.isRequired, | ||
data: PropTypes.array.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't quite tell what group is from the code, seems like an array based on this line
[ { group: [array of something?] values: [number, number] } ]
PropTypes.arrayOf(
PropTypes.shape({
group: PropTypes.arrayOf(??),
values: PropTypes.arrayOf(PropTypes.number),
}),
)
className: PropTypes.string, | ||
metrics: PropTypes.arrayOf(PropTypes.string).isRequired, | ||
groups: PropTypes.array.isRequired, | ||
data: PropTypes.object.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we be more specific about the shape here? I can't quite figure out what group is from the code, seems like an array based on this line
{ my_metric: [ { group: [array of something?] values: [number, number] } ] }
PropTypes.objectOf(
PropTypes.arrayOf(
PropTypes.shape({
group: PropTypes.arrayOf(??),
values: PropTypes.arrayOf(PropTypes.number),
}),
),
)
Thanks you for catching the |
* extract TTestTable into another file. * Move into directory. * update proptypes (cherry picked from commit 68e7794)
* extract TTestTable into another file. * Move into directory. * update proptypes
* extract TTestTable into another file. * Move into directory. * update proptypes
slice
andformData
TTestTable
component into another file.@williaster @conglei