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

Patient Timeline Improvements #2950

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

Luke-Sikina
Copy link
Member

@Luke-Sikina Luke-Sikina commented Dec 23, 2019

Fix cBioPortal/cbioportal#6882

Describe changes proposed in this pull request:

  • Add TSV download
  • Update version of clinical-timeline to 0.0.30. This comes with the following enhancements:
    • Cut off circles
      • Issue: mousing over points on the bottom lane of the
        timeline caused them to be cut off when they expanded.
      • Fix: added 3 pixels to the height of the svg

Test with

https://deploy-preview-2950--cbioportalfrontend.netlify.com/patient?caseId=P29&studyId=lgg_ucsf_2014#navCaseIds=lgg_ucsf_2014:P01,lgg_ucsf_2014:P02,lgg_ucsf_2014:P04,lgg_ucsf_2014:P05,lgg_ucsf_2014:P06,lgg_ucsf_2014:P07,lgg_ucsf_2014:P08,lgg_ucsf_2014:P09,lgg_ucsf_2014:P10,lgg_ucsf_2014:P11,lgg_ucsf_2014:P12,lgg_ucsf_2014:P13,lgg_ucsf_2014:P15,lgg_ucsf_2014:P16,lgg_ucsf_2014:P17,lgg_ucsf_2014:P18,lgg_ucsf_2014:P21,lgg_ucsf_2014:P24,lgg_ucsf_2014:P25,lgg_ucsf_2014:P26,lgg_ucsf_2014:P27,lgg_ucsf_2014:P28,lgg_ucsf_2014:P29

Also possible to test on private portals:

localStorage.clear()
localStorage.netlify = 'deploy-preview-2950--cbioportalfrontend'

And refresh the page

package.json Outdated Show resolved Hide resolved
@inodb
Copy link
Member

inodb commented Jan 31, 2020

Nice work!

Not sure if this should be part of the work but i noticed some issues with how the ticks behave at different levels of zoom. It works for the default example here: https://rawgit.com/cBioPortal/clinical-timeline/master/index.html. But not for the other example JSON we exchanged before (see slack convo). Occasionally the number of ticks calculation seems really off. E.g. if I zoom in to a really tiny part I can never get the ticks to show up every 1 day for instance (smallest tick size seems to be 10 days?).

Screen Shot 2020-01-31 at 1.51.55 PM.png

Ideally the ticks would be readjusted 👆

It might be that the JSON example triggers a particular corner case

@jjgao
Copy link
Member

jjgao commented Feb 9, 2020

How can I test this one?

@inodb
Copy link
Member

inodb commented Feb 11, 2020

Hey @Luke-Sikina - is this the PR we should be testing? The deploy preview on Netlify doesn't seem to be working

@Luke-Sikina Luke-Sikina force-pushed the timeline-improvements branch 3 times, most recently from 52deedc to c2b85d0 Compare February 12, 2020 17:38
@inodb
Copy link
Member

inodb commented Feb 13, 2020

Hi @Luke-Sikina - somehow the zoom doesn't seem to be working for me?

https://deploy-preview-2950--cbioportalfrontend.netlify.com/patient/summary?caseId=P04&studyId=lgg_ucsf_2014

The TSV download seems to be working well 👍

@inodb
Copy link
Member

inodb commented Feb 13, 2020

@Luke-Sikina Noticed some missing parts in the TSV:

Might be good to follow the input spec for the TSV i.e. the data that goes into cBioPortal can be the same format as if u download it from the timeline:

https://docs.cbioportal.org/5.1-data-loading/data-loading/file-formats#timeline-data

I see for instance for P04 that the field Medical Therapy is missing:

https://deploy-preview-2950--cbioportalfrontend.netlify.com/patient/summary?caseId=P04&studyId=lgg_ucsf_2014

The raw input data for that study is here:

https://github.com/cBioPortal/datahub/tree/master/public/lgg_ucsf_2014

I'm not sure if there's a reason for having each track in a separate TSVs there. For a single patient that's prolly not necessary

UPDATE: I don't think this is a show stopper tho. If zoom works that's great and some form of TSV download is better than no TSV download at all

@Luke-Sikina Luke-Sikina force-pushed the timeline-improvements branch 2 times, most recently from 5f22201 to 764bdf8 Compare February 13, 2020 20:32
@inodb
Copy link
Member

inodb commented Feb 13, 2020

Thanks a ton @Luke-Sikina! Looks great!

Some things I found:

  • the ticks look good now but it might be nice to use .y.m.d formatting of the ticks when they get > 1 month. E.g. here you see 1200+ days, which would be more comprehensible as something like 1y3m15d. Not sure how tricky this is. There should be a function that in the clinical timeline that does that calculation

    Screen Shot 2020-02-13 at 3 52 33 PM

  • would be nice if the brush had a different color than black e.g. gray or blueish. Similar to the study view

Screen Shot 2020-02-13 at 3 57 30 PM

I'm not too worried about most of these. We can file them as a new issue. The first two and the last issue would prolly be good to fix before release. If it turns out some of those are really tricky to fix we can maybe do some workaround. Like enable zoom on demand and indicate that it's in beta or something

@Luke-Sikina Luke-Sikina force-pushed the timeline-improvements branch 2 times, most recently from 93d7ec0 to 5c8af1c Compare February 18, 2020 16:41
@Luke-Sikina
Copy link
Member Author

@inodb I think I've fixed all the issues. The PR for the timeline repo is here: cBioPortal/clinical-timeline#122

One small note: using patient P04 to test trimming wont work well. There was an issue where the initial timeline wasn't getting trimmed, but when you zoomed in and out the timeline would trim. I fixed that, but P04 was an example where it was happening, so now it doesn't. P18 from the same study should be a better candidate.

@jjgao jjgao requested a review from tmazor February 25, 2020 18:58
@tmazor
Copy link
Contributor

tmazor commented Feb 27, 2020

@Luke-Sikina There are a few issues with download of the timeline images:

  1. If I have zoomed in, the "zoom out" text is visible in the download. Can it be removed?
  2. The zig-zag (when present) doesn't download nicely
    image
  3. The time labels overlap each other a little bit in the website, and a lot when downloaded
    image

The above were tested with P17 (the last one after expanding the zig-zag)
https://deploy-preview-2950--cbioportalfrontend.netlify.com/patient/summary?caseId=P17&studyId=lgg_ucsf_2014

@inodb inodb assigned tmazor and unassigned inodb Mar 10, 2020
@Luke-Sikina Luke-Sikina force-pushed the timeline-improvements branch 3 times, most recently from 005db3f to 3f6f0b3 Compare March 10, 2020 17:45
@Luke-Sikina
Copy link
Member Author

@tmazor
This is ready for you to test again: https://deploy-preview-2950--cbioportalfrontend.netlify.com/patient?studyId=lgg_ucsf_2014&caseId=P17

The zooming logic has been improved, nothing breaks when you zoom on the edge, and single click zooming works as you expected.
I couldn't fix the pdf issues- those were already broken, so we'll look to fix them in another pull request

@tmazor
Copy link
Contributor

tmazor commented Mar 17, 2020

@Luke-Sikina The single click zoom is great!

A few things:

  • In the link you provided above, I clicked to zoom on the right side of the plot (so the zig-zag timeline compression was still visible), and then clicked on the zig-zag to expand, at which point the "zoom out" button disappears.
  • It appears to only be possible to zoom in once. Can we enable a user to zoom in and then zoom in some more?
  • For this case: https://deploy-preview-2950--cbioportalfrontend.netlify.com/patient?studyId=lgg_ucsf_2014&caseId=P18
    If I click-and-drag to zoom in from 0-1y, what appears is only 0-~6months

@inodb
Copy link
Member

inodb commented Mar 23, 2020

Thanks so much for reviewing @tmazor !

We are hoping to release this soon. I thought it might be ok to release this without your last two comments fixed and follow up on those later. I've filed the last two things as separate issues: cBioPortal/cbioportal#7325 cBioPortal/cbioportal#7326

Let me know if you think that makes sense and if there's anything else that is required to be fixed before rolling out. Thank you!

@inodb inodb force-pushed the timeline-improvements branch 2 times, most recently from 19fc71b to dc96ad9 Compare April 8, 2020 14:32
@Luke-Sikina Luke-Sikina force-pushed the timeline-improvements branch 2 times, most recently from fc6fc9b to b8a2c70 Compare April 8, 2020 18:17
@inodb inodb force-pushed the timeline-improvements branch 2 times, most recently from e51c117 to 46ecb4b Compare April 13, 2020 18:30
- Enable zoom
- Add TSV download
- Update version of clinical-timeline to 0.0.22. This comes with the following enhancements:
  - Zoom + Trim
    - Issue: when trimming the timeline, zooming would zoom on the
    wrong region
    - Root cause: start and end of zoom region were determined
    according to untrimmed timeline. This was happening inside the
    d3 library itself
    - Fix: take the point the the brush generates, determine its
    placement on the untrimmed timeline, find the nearest tick
    on the trimmed timeline, replace the point with that.
  - Cut off circles
    - Issue: mousing over points on the bottom lane of the
    timeline caused them to be cut off when they expanded.
    - Fix: added 3 pixels to the height of the svg
  - Ruler annotation
    - Problem
      - Month and day annotations for large values were hard to
      understand at a glance
    - Solution
      - Write larger values in `y{}m{}d{}` form
      - Ex: "1860d" => "5y1m5d"
  - Zoom
    - Problem
      - The start of the zoom is fixed now, but region ends too soon
    - Fix
      - Adjusted the zoom region when trimmed
  - Fix Scroll/drag text renders twice
  - Fix "Click + drag to zoom" text not part of right element,
  flickers
- Change zoom brush overlay color
- Got rid of translate(null,null) errors when zooming in

See also: cBioPortal/clinical-timeline#122
@inodb
Copy link
Member

inodb commented Apr 16, 2020

Decided to disable zoom for now because we still found issues unfortunately. We will merge the other fixes & updates

Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

Decided to disable zoom for now because we still found issues unfortunately. We will merge the other fixes & updates 👍

@inodb inodb merged commit 7984801 into cBioPortal:master Apr 16, 2020
inodb added a commit to inodb/cbioportal-frontend that referenced this pull request Jan 12, 2022
…ents

Patient Timeline Improvements

Former-commit-id: 7984801
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants