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

Study view km plot optimization #4520

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Conversation

dippindots
Copy link
Member

@dippindots dippindots commented Feb 17, 2023

Fix cBioPortal/cbioportal#9988
Fix cBioPortal/cbioportal#8378

Changes proposed in this pull request:

  • Keep old implementation for comparison page/comparison tab survival plot

  • Only apply binning when the sample size is larger than the threshold (default 1000, we can change this into a configurable value later)
    image

  • Logic for binning: flooring month to integer (e.g. flooring 100.2 or 100.4 to 100) and count any events or censored data that get aggregated

  • Add information into tooltip to show the count for events and censored

Other comments:

  • Change text in the tooltip:
    • Time of event: 46-47 months -> Events during [46,47) months. Use brackets/parenthesis notation here depending on how you are binning: https://stackoverflow.com/a/4396303
    • Patients with an event:
    • Censored patients:
    • Newline
    • % event free at end of months (46 months) -> % event free at interval end:
    • Patients at risk at interval end:
    • Screen Shot 2023-02-20 at 3 46 40 PM
  • Indicate in info icon that events were floored to the nearest month
  • Indicate the unit for the x axis in the info icon

Follow-up ticket:

cBioPortal/icebox#483

@dippindots dippindots self-assigned this Feb 17, 2023
@dippindots dippindots force-pushed the fix-9988 branch 3 times, most recently from 4e9a4b0 to b4d52a0 Compare February 17, 2023 19:29
@dippindots dippindots requested a review from alisman February 17, 2023 20:42
@inodb
Copy link
Member

inodb commented Feb 20, 2023

Thanks @dippindots - this looks great!

Maybe a few tweaks to the text and ordering here:

Screen Shot 2023-02-20 at 3 46 40 PM

  • Time of event: 46-47 months -> Events during [46,47) months. Use brackets/parenthesis notation here depending on how you are binning: https://stackoverflow.com/a/4396303
  • Patients with an event:
  • Censored patients:
  • Newline
  • % event free at end of months (46 months) -> % event free at interval end:
  • Patients at risk at interval end:

It might make sense to highlight the entire bin when mousing over (so one horizontal line). Currently, it highlights points and it's a little hard to see that it's showing an interval:

Screen Shot 2023-02-20 at 4 15 57 PM

Additional comments from Niki:

  • Can we indicate somewhere (maybe in info icon) that events were floored to the nearest month?
  • Can we also indicate the unit for the x axis somewhere (also in the info icon)?

@tmazor
Copy link
Contributor

tmazor commented Feb 21, 2023

This is great! I agree with all of Ino's comments above, and I also noticed that the tooltips will extend off the page:
image

Can you make the tooltips appear to the left of the chart when the chart is on the right side of the page, like pie charts do?
image

}

export function getLineDataFromScatterData(data: ScatterData[]): any[] {
let chartData: any[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems it would be simple enough to type this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dippindots can we type this?

@alisman
Copy link
Collaborator

alisman commented Mar 9, 2023

@dippindots lets bring this up at standup today to figure out what's next.

@alisman alisman merged commit 3f0ac3d into cBioPortal:master Apr 27, 2023
@dippindots dippindots added the performance This PR is related to a performance issue label Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance This PR is related to a performance issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize rendering of the study view KM charts Adjustments to custom "Survival" KM plots
4 participants