-
Notifications
You must be signed in to change notification settings - Fork 5
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: conditionally enable regression methods based on term type and s… #2586
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
|
||
Fixes: | ||
- conditionally enable regression methods based on term type and show customized Regression chart button if a single method is available |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -582,9 +582,15 @@ const defaultCommonCharts: isSupportedChartCallbacks = { | |
summary: () => true, | ||
matrix: () => true, | ||
regression: () => true, | ||
linear: () => true, | ||
logistic: () => true, | ||
cox: () => true, | ||
// linear/logistic/cox can be considered "child types" for regression, their availability can be separately determined to be more user friendly | ||
linear: ({ cohortTermTypes }) => { | ||
return cohortTermTypes.numeric > 0 // numeric term present and could be used as linear outcome | ||
}, | ||
logistic: () => true, // consider both numeric & categorical can be logistic outcome | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can add 'binary' outcome |
||
cox: ({ cohortTermTypes }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can add example, 'time to event' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure what u mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can add examples for comments I meant:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. survival and condition terms are time-to-event variables and can be used as Cox outcome. these are facts outside of our code thus not really explained |
||
// requires either survival or condition term as cox outcome | ||
return (cohortTermTypes.survival || 0) + (cohortTermTypes.condition || 0) > 0 | ||
}, | ||
facet: () => true, | ||
survival: ({ cohortTermTypes }) => cohortTermTypes.survival > 0, | ||
cuminc: ({ cohortTermTypes }) => cohortTermTypes.condition > 0, | ||
|
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 feel in our codebase we should refrain from using alphabets like 'm' to assign values. I understand that it's within the scope of that block but it's in several places in the codebase and searching becomes difficult if used in several scripts.
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.
making an improvement for this and backend
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.
done please check again