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

RFC69 : survival chart with landmark events and hazard ratio #4740

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

TJMKuijpers
Copy link
Contributor

RFC69: Add hazard ratio and landmarks in the survival plot

For clinical trials, it is customary to report hazard ratios and/or landmark values in survival plots. Currently, survival plots rendered by cBioPortal do not show hazard ratio or landmark information.

This PR is the implementation of RFC69. The new survival chart can be activated by setting &featureFlag=SURVIVAL_PLOT_EXTENDED.

Implemented functions

Number at risk
Most scientific papers show a survival plot together with the number at risk table. We have added this option to the survival plot and show the number of risks below the figure (number at risk at every tick on the x-axis).

Landmark lines

The user activates the landmarks option via a checkbox (by default, the landmark input field is disabled). The user can add values (comma or space separated) that are translated into vertical lines (dashed) in the survival plot.

Underneath the number at risk table, we show a new table with the percentage of patients at that specific landmark point.
Figure2_Landmarklines

Hazard ratios

The user activates the hazard ratio option via a checkbox. The user selects the Control group to serve as a reference (denominator) in the hazard ratio by dragging the Control group to the leftmost position in the Groups menu section. The hazard ratios for each experimental group ( with a 95% confidence interval and p-value) appear below the respective entries in the plot legend.
Figure1_HazardRatio

Implemented tests

Unit test for the new functions

  • Test for the hazard ratio calculations (based on the survival R package for the ground truth)
  • Test for the log-rank test to calculate the lower and upper confidence intervals (based on the survival R package)

Implemented screenshot tests:

  • Survival chart with hazard ratio table and landmark line disabled
  • Survival chart with landmark event at time point 20
  • Survival chart with hazard ratio table
  • Survival chart with hazard ratio table and landmark event at time point 20

@inodb inodb requested review from alisman and dippindots September 21, 2023 14:40
@@ -133,7 +133,7 @@ export default abstract class ComparisonStore extends AnalysisStore
@observable public adjustForLeftTruncation = true;

constructor(
protected appStore: AppStore,
public appStore: AppStore,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one should be made protected again and the featureFlag for the extended survival plot should be shared with in a better way.

@TJMKuijpers TJMKuijpers marked this pull request as ready for review September 25, 2023 07:51
@@ -317,6 +319,7 @@ export default class Survival extends React.Component<ISurvivalProps, {}> {
style={{ paddingBottom: 0 }}
>
{getStatisticalCautionInfo()}
{getHazardRatioCautionInfo()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe do the same for getStatisticalCautionInfo while we're at it. (Function component)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also changed it for getStatisticalCautionInfo and also updated /groupComparison/ClinicalData.tsx since it also calls getStatisticalCautionInfo

return _.sortBy(curveData, d => d.months);
});

const groupsTte = sortedCurves.map(curveData => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you improve this name? No idea what Tte means. Ir Ev.

controlGroup: string,
...survivalCurves: SurvivalCurveItem[][]
) {
const allEvents = _.sortBy(_.flatten(survivalCurves), s => s.months);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is important business logic here so comments about what we're doing would be helpful to devs trying to understand the basics

@computed get tableRowsHazardRatio() {
if (this.hazardRatioGroups !== null) {
return (
<React.Fragment>
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think you need Fragment here since you are just returning a single element (table)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the <React.Fragment>

const SURVIVAL_DOWN_SAMPLING_THRESHOLD = 1000;

@observer
export default class SurvivalChartExtended
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should discuss reason for doing this as extension rather than just a new component. the "Extension" kind of hides the true nature of this component. lets discuss.

@alisman
Copy link
Collaborator

alisman commented Oct 4, 2023

@pvannierop I wanted to open a discussion about the practice here of copying an existing component in order to "extend it". Based on discussion with @TJMKuijpers, I now think I understand that <SurvivalExtended> is intended to fully replace <SurvivalChart> and so it's not really an "extension." It's just adding to adding to the existing component (right?). The issue then is that by copying the component in order to work on it, we leave ourselves vulnerable to losing concurrent edits to the original component. If we want to use a feature flag to introduce this work then I think it should be be done via "mode" of the original component. Alternatively, we don't use a feature flag in this case. We just keep the feature branch open. Since this is a pretty isolated feature, I don't see a big cost to that.

@TJMKuijpers TJMKuijpers force-pushed the survival-plot-landmarks branch 7 times, most recently from 987a847 to ef59d55 Compare October 13, 2023 14:51
Added the input field that reacts on user defined input

Implemented multiple landmark lines

Add the group information as label below chart

landmark line functionality complete

Dynamic calculation of groups for axis

Implemented hazard ratio

updated survival chart and tests

show hazard ratio between every group

Updated test

Updated number at risk and legend

Updated styling

Updated statistics in survival chart

updated CI calculations

Updated SV plot

Updated styling labels

Added feature flag

updated files

fix behavior disable landmark input

updated hazard ratio table style

updated style table

Updated screenshot tests for extended survival chart

removed console.log

getStatisticalCautionInfo and getHazardRatioCautionInfo changed to functional component

Updated PR

Updated legend style for hazard ratio legend

Remove hazard ratio warning when feature flag is disabled

Run Prettier

Update the border chart for the default survival chart

Removed feature flag for survival chart

Updated screenshots

Updated padding to fix screenshot

Decreased whitespace between table and figure

Update reference screenshot

update localdb screenshots ref

Updated font family landmark line labels

Updated the test for survival chart

localdb tests
@TJMKuijpers TJMKuijpers force-pushed the survival-plot-landmarks branch from afcbd50 to a2f223e Compare October 13, 2023 16:17
@TJMKuijpers
Copy link
Contributor Author

@inodb updated the labels for the landmark line groups (had to change CBIOPORTAL_VICTORY_THEME.chart.fontFamily, to CBIOPORTAL_VICTORY_THEME.legend.style.labels.fontFamily). Also updated the reference screenshots so the surival chart tests should pass.
ImageSV1

@inodb inodb added feature and removed enhancement labels Oct 16, 2023
@inodb inodb merged commit 2e24485 into cBioPortal:master Oct 16, 2023
haynescd pushed a commit that referenced this pull request Nov 15, 2023
Add the group information as label below chart
landmark line functionality
Dynamic calculation of groups for axis
Implement hazard ratio
show hazard ratio between every group
Remove hazard ratio warning when feature flag is disabled
Removed feature flag for survival chart
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants