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

Left truncation adjustment for KM plot #4324

Merged
merged 10 commits into from
Nov 10, 2022
Merged

Conversation

dippindots
Copy link
Member

@dippindots dippindots commented Jul 11, 2022

Please using links:

What is in this pr:

  • Left truncation adjustment algorithm for KM plot
  • Update unit tests
  • Update tooltip for KM plot
  • Add number of risk in the tooltip when hover over the data point in the KM plot
  • Show how many patients get excluded when applying left truncation algorithm

@inodb
Copy link
Member

inodb commented Jul 22, 2022

@dippindots really nice work!

For study view:

  • I like the banner re adjustment for left truncation! Wondering though if we should make it a bit smaller. Maybe put "Adjust for left truncation (xx patients excluded) [info icon]" and then when you mouse over we can put the rest of the text. This would help in getting the survival chart to look square. Both info icons (in the header of the chart and this one) can have the same/similar explanatory text
  • Adjust background to be white instead of yellow, I think it is a little too eye drawing at the moment
  • Change the explanatory text in the tooltip to something like: "Patients enter a study when they are profiled, which can be months or even years after the initial diagnosis. To mitigate the effects of left truncation one can enable the risk-set adjustment method as described in Brown et al (2022). This involves adjusting patients at risk at time t to date of sequencing rather than date of diagnosis (xx patients are excluded because they passed away before their biopsies were sequenced)". I know it's a lot of text so need to figure out how we can display it. Alternatively we can link to the docs site or something with a more elaborate explanation of the method

Results/Comparison View View:

  • Can we add a checkbox here as well?

@brookemastrog
Copy link

@dippindots Excellent work! This is amazing.

My only comment is that we should probably adjust this warning: "Kaplan-Meier estimates do not account for the lead time bias introduced by the inclusion criteria for the GENIE BPC Project." as we can adjust for some curves at this point. Some of these warnings also come from the data files themselves, so we need to figure out a way to correctly display these warnings.

In the space where the left truncation adjustment is described, it would be convenient/ consistent to have a warning that the curve is not corrected. In both the smaller plots on the summary page and in the comparison view.

Alternatively, we could change the warning to something more general such as: "Not all Kaplan-Meier estimates account for the lead time bias introduced by the inclusion criteria for the GENIE BPC Project."

@inodb
Copy link
Member

inodb commented Aug 18, 2022

@dippindots for this tooltip:
Screen Shot 2022-08-18 at 11 23 22 AM

It might be good to:

  • Remove Adjusted from the title now that you have a more elegant solution
  • Revert the text in the tooltip of the info icon in the header back to what it was. With one minor change. That is: if no left truncation data is available set the text to:Overall survival in months from first index diagnosis. Kaplan-Meier estimates do not account for the lead time bias introduced by the inclusion criteria for the GENIE BPC Project.. If it is available remove the last sentence: Overall survival in months from first index diagnosis. (since we are accounting for it now)

In the left truncation tooltip:

Screen Shot 2022-08-18 at 11 32 22 AM

  • It says xx patients. This should be 106, the same number as:
    Screen Shot 2022-08-18 at 11 33 34 AM

On the comparison page:

Screen Shot 2022-08-18 at 11 34 40 AM

  • Let's only keep the top "Adjust for left truncation" checkbox. That one looks good
  • It would be good if we could remove all the "Kaplan-Meier estimates do not account for the lead time bias introduced by the inclusion criteria for the GENIE BPC Project." in these pages, since we are adjusting for it in case of OS. Some of this might be in data, so not sure how easy. I guess we could just do a hardcoded search for that phrase and remove it in the frontend only when "left truncation adjustment" is possible
  • Why are there 105 patients excluded instead of 106 on the comparison page? I just compared TP53 altered vs unaltered, so I don't fully get why there would be one patient less in that comparison compared to Study View. Any clue?

Does that make sense? Thanks!

@dippindots dippindots marked this pull request as ready for review October 24, 2022 14:30
@dippindots dippindots force-pushed the fix-9030 branch 2 times, most recently from 4b660e5 to d4659fb Compare October 31, 2022 13:53
@dippindots dippindots changed the title (Do not merge) left truncation demo Left truncation adjustment for KM plot Oct 31, 2022
readonly survivalPlotData = remoteData<SurvivalType[]>({
await: () => [
this.survivalData,
this.selectedPatientKeys,
this.survivalPlots,
this.survivalEntryMonths,
],
invoke: async () => {
return this.survivalPlots.result.map(obj => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i can see that this isn't new but it's a little wrong that we are mutating survivalPlots here, tacking on the survival data. it's suprising to me that typescript allows this. in other words, that the survivalPlot object will accept survivalDataWithoutLeftTruncation as a property. better to just make consumers look it up using plot id?

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed it by not changing the property in place, is this looks good? 313fbab

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 this is great either. it doesn't really make sense to collapse these two types in this way. maybe just key the survivalPlotData by id and then look it up when we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to a search by id approach in commit : dbdd28d

@alisman
Copy link
Collaborator

alisman commented Oct 31, 2022

Style tweaks. Lets remove alert styling. For one thing this isn't really semantically an "alert". Just a div with some padding and no border will be good.
image

@alisman
Copy link
Collaborator

alisman commented Oct 31, 2022

Tweaks for comparison tab. Lets move the the checkbox and label outside of the dotted chart container like so. Again, remove the alert class. In general, we shouldn't be stuffing controls into the charts. Sometimes we do so to save space but I don't think it's a good habit.

image

@dippindots
Copy link
Member Author

@alisman Thanks for reviewing this, here is the commit that addresses the most recent comments: b59c900

@alisman alisman merged commit ffcbea9 into cBioPortal:master Nov 10, 2022
@dippindots dippindots added the cl-prototype Prototype section in changelog. New features not ready for production use label Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cl-prototype Prototype section in changelog. New features not ready for production use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants