Fix surivival chart for large cohort label overlap (number at risk) #4833
Conversation
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@onursumer would be nice if you can provide some code feedback. I will work on updating the tests. |
| @observable visible = false; | ||
|
|
||
| // determine whether visibility is controlled | ||
| // by a store component or by local state | ||
| @computed get visibilityState() { | ||
| return this.visible; | ||
| } | ||
| @action setVisibilityState(visible: boolean | undefined) { | ||
| if (visible !== undefined) { | ||
| this.visible = !!visible; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't see a reference to any of these variables (visible, visibilityState, setVisibilityState). Do we actually use them somewhere?
There was a problem hiding this comment.
Removed this part since it became obsolete.
| @action.bound | ||
| calculateLabelWidth(text: number, fontFamily: string, fontSize: number) { |
There was a problem hiding this comment.
We should not mark this function as an action, because it does not modify any observable values. Also, we can probably use the getTextWidth function instead of writing a new one.
There was a problem hiding this comment.
I moved calculateLabelWidth to the SurvivalUtils so it becomes a function.. I checked and there is a difference in the width if we calculate it for a label or just the plain text. I believe the getTextWidth function would still allow for overlap in the survival chart.
|
|
||
| return valueAtAxis; | ||
| const addLabelsForTimePoint = ( |
There was a problem hiding this comment.
It would be nice if we can add a comment somewhere in this function to describe how we decide which labels to hide.
Also, we had a similar issue with the lollipop plot labels. I was wondering if we could generalize that logic and use it here as well. It would probably require some refactoring though. So maybe okay to have two separate implementations for now.
There was a problem hiding this comment.
Added the explanation on when labels are shown/hidden. Will have to compare to logic of caculateTicks
TJMKuijpers
force-pushed
the
custom-interval-number-at-risk
branch
from
248cb28
to
46e478a
Compare
TJMKuijpers
force-pushed
the
custom-interval-number-at-risk
branch
from
6982ff0
to
3a964a0
Compare
TJMKuijpers
force-pushed
the
custom-interval-number-at-risk
branch
3 times, most recently
from
ff2ce7b
to
fd3da6d
Compare
Logic: when labels overlap, draw label at index 0 and skip all labels at uneven index Updated PR after code review Updated util function Remove commented code which is obsolete code cleanup Code cleanup of SurvivalUtil updated survival chart resolved rebase conflict update screenshots
TJMKuijpers
force-pushed
the
custom-interval-number-at-risk
branch
from
4988f46
to
46ce32e
Compare
dippindots
changed the title
This PR updates the number at risk table behavior in the survival chart for large cohorts (>10.000 samples). The current number-at-risk table plots the number at risk for every time point (under the x-axis, see Figure 1). However, when groups contain many samples, the labels overlap and become unreadable.
We have added a function that calculates whether the labels will overlap. If labels overlap, the number at risk table is updated to remove the overlapping labels (Figure 2). If no labels overlap, we default to the default behavior of the number of risk tables.
Figure 1: old number at risk table for large cohorts when labels overlap
Figure 2: new number at risk table for large cohorts when labels overlap