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: conditionally enable regression methods based on term type and s… #2586

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 46 additions & 9 deletions client/mass/charts.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,42 @@ class MassCharts {
!d.hide && this.state.currentCohortChartTypes.includes(d.chartType) ? '' : 'none'
)
}

generateRegressionButtonObject(state) {
// the default obj for regression, this obj is suitable for a button that will launch a menu with multiple regression methods
const obj = {
label: 'Regression Analysis',
chartType: 'regression',
clickTo: this.loadChartSpecificMenu
}
/* following detects if the ds has just one method, if so will customize obj{} so that the button directly show the available method name, and click on it to directly launch the ui without showing a menu, and will not trigger makeChartBtnMenu()
TODO move this logic to regression makeChartBtnMenu() to keep chart-specific logic out of here, but there lacks a way for makeChartBtnMenu to customize obj.label
*/
const lst = getCurrentCohortChartTypes(state)
if (!lst.includes('regression')) return obj // regression is hidden. still return obj to maintain proper charts array and the button will be hidden later
const availableMethods = []
if (lst.includes('linear')) availableMethods.push('linear')
if (lst.includes('logistic')) availableMethods.push('logistic')
if (lst.includes('cox')) availableMethods.push('cox')
if (availableMethods.length > 1) return obj // more than 1 regression methods. do not modify original obj
if (availableMethods.length == 1) {
// has only one method. customized button label and click behavior
obj.label =
(availableMethods[0] == 'linear' ? 'Linear' : availableMethods[0] == 'cox' ? 'Cox' : 'Logistic') + ' Regression'
obj.clickTo = () => {
this.dom.tip.hide()
this.prepPlot({
config: {
chartType: 'regression',
regressionType: availableMethods[0],
independent: []
}
})
}
}
// no method (exception?) this is handled by regression makeChartBtnMenu
return obj
}
}

export const chartsInit = getCompInit(MassCharts)
Expand Down Expand Up @@ -72,6 +108,8 @@ function getChartTypeList(self, state) {
each char type will generate a button under the nav bar
a dataset can support a subset of these charts

allow some chart button objects to be dynamically generated based on state

design goal is that chart specific logic should not leak into mass UI

design idea is that a button click will trigger a callback to do one of following things
Expand Down Expand Up @@ -114,8 +152,12 @@ function getChartTypeList(self, state) {

.updateActionBySelectedTerms:
optional callback. used for geneExpression and metabolicIntensity "intermediary" chart types which do not correspond to actual chart, but will route to an actual chart (summary/scatter/hierclust) based on number of selected terms. this callback will update the action based on selected terms to do the routing

TODO fixed order of buttons. may allow to customize order
*/
const [logged, site, user] = getProfileLogin() //later on replace with jwt login

const [logged, site, user] = getProfileLogin() // XXX later on replace with jwt login
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add comments about this login method or do we have any documentation where that is available?

Copy link
Collaborator Author

@xzhou82 xzhou82 Jan 10, 2025

Choose a reason for hiding this comment

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

no need; this is a simple temp fix to mimic profile user roles and will be deleted later with proper implementation
to add that the 'proper' implementation is on the backend via the isSupportedChartTypes() overrides and no longer on the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks for explaining


const buttons = [
////////////////////// PROFILE PLOTS START //////////////////////
{
Expand Down Expand Up @@ -178,25 +220,20 @@ function getChartTypeList(self, state) {
clickTo: self.loadChartSpecificMenu
},
{
// should only show for official dataset, but not custom
label: 'Cumulative Incidence',
chartType: 'cuminc',
clickTo: self.showTree_select1term,
usecase: { target: 'cuminc', detail: 'term' }
},
{
// should only show for official dataset, but not custom
label: 'Survival',
chartType: 'survival',
clickTo: self.showTree_select1term,
usecase: { target: 'survival', detail: 'term' }
},
{
// should only show for official dataset, but not custom
label: 'Regression Analysis',
chartType: 'regression',
clickTo: self.loadChartSpecificMenu
},

self.generateRegressionButtonObject(state), // returns an object like other buttons but tailored by current state

{
label: 'Sample Matrix',
chartType: 'matrix',
Expand Down
11 changes: 4 additions & 7 deletions client/plots/regression.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,18 +210,15 @@ export function makeChartBtnMenu(holder, chartsInstance) {
/*
holder: the holder in the tooltip
chartsInstance: MassCharts instance
termdbConfig is accessible at chartsInstance.state.termdbConfig{}
mass option is accessible at chartsInstance.app.opts{}
*/

const lst = [
const allMethods = [
{ label: 'Linear', type: 'linear' },
{ label: 'Logistic', type: 'logistic' },
{ label: 'Cox', type: 'cox' }
]

for (const { label, type } of lst) {
if (!chartsInstance.state.currentCohortChartTypes.includes(type)) continue
const useMethods = allMethods.filter(i => chartsInstance.state.currentCohortChartTypes.includes(i.type))
if (useMethods.length == 0) return holder.append('div').text('Error: no methods available')
for (const { label, type } of useMethods) {
holder
.append('div')
.attr('class', 'sja_menuoption sja_sharp_border')
Expand Down
3 changes: 2 additions & 1 deletion release.txt
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
22 changes: 19 additions & 3 deletions server/src/termdb.server.init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,14 +581,29 @@ const defaultCommonCharts: isSupportedChartCallbacks = {
dictionary: () => true,
summary: () => true,
matrix: () => true,

/*
parent type: regression
child types: linear/logistic/cox
- if parent is disabled, all child types are not accessible
- when parent is accessible, availability of each child type is individually calculated based on data types and allows for ds override for customization
*/
regression: () => true,
linear: () => true,
logistic: () => true,
cox: () => true,
linear: ({ cohortTermTypes }) => cohortTermTypes.numeric > 0, // numeric term present and could be used as linear outcome
logistic: () => true, // always enabled by default because: numeric/categorical/condition terms could all be used as outcome. later we will support custom samplelst term of two groups as outcome. a ds can provide an override to hide it if needed
cox: ({ cohortTermTypes }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can add example, 'time to event'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure what u mean

Copy link
Contributor

Choose a reason for hiding this comment

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

can add examples for comments I meant:
I can add them in if you'd like.

linear: ({ cohortTermTypes }) => {
    // Check if there is at least one numeric term present, which can be used as a linear outcome
    // Example: outcome is a continuous variable.
    return cohortTermTypes.numeric > 0;
},
logistic: () => true, 
    // Allow both numeric and categorical terms to be considered as logistic outcomes
    // Example: Predicting whether a patient has a disease (yes/no), where the outcome is a binary variable
},
cox: ({ cohortTermTypes }) => {
    // Ensure there is at least one survival or condition term present, which is required for a Cox outcome
    // Example: Analyzing the time until a patient relapses after treatment, where the outcome is the time until the event occurs
    return (cohortTermTypes.survival || 0) + (cohortTermTypes.condition || 0) > 0;
},

// 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,

/*
parent type: sampleScatter
child type: dynamicScatter
*/
sampleScatter: ({ ds, cohortTermTypes }) => {
// corresponds to the "Scatter Plot" chart button. it covers both premade scatter plots, as well as dynamic scatter input ui on clicking the "Scatter Plot" chart button
if (ds.cohort.scatterplots) return true
Expand All @@ -605,6 +620,7 @@ const defaultCommonCharts: isSupportedChartCallbacks = {
if (cohortTermTypes.numeric > 1) return true // numeric is always prefilled for convenience, does not have to check if property exists
return false
},

genomeBrowser: ({ ds }) => {
// will need to add more logic
if (ds.queries?.snvindel || ds.queries?.trackLst) return true
Expand Down