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

Add GWAS catalog to PheWeb #112

Closed
wants to merge 15 commits into from

Conversation

abought
Copy link
Member

@abought abought commented Sep 25, 2018

Ticket: #17

Purpose

Add a GWAS catalog plot to PheWeb, based on an upcoming LZ.js release

screen shot 2018-09-25 at 3 05 44 am

TODO

Recommendations for discussion

  • Add test data that has a sample region with SNPs in the catalog (Peter)
  • Goncalo suggests making the GWAS catalog annotation track optional. (eg, rely solely on labels in the display options menu). Thoughts?

Known issues

  • Identify whether pointer-events:none is still needed, and if removal is a regression.
  • Since this updates LZ.js to the newest version, generally review app for breakages
  • Plot height is a bit too tall; let Peter sort that out.

Summary of changes

  • Replace in-repo LZ.js with a version from a CDN
  • DRY layouts to use extension mechanism
  • Region plots now use "association_catalog" instead of "standard_association" for their base plot layout (and appropriate child layouts, with modifications)
  • Change association source: remove override of a deprecated function, in favor of explicit namespaced field requests (revisit this)
  • Change LD source to eliminate redundant code- use subclassing for maintainability going forward

How to test this

  • Version 0.9.0 has not yet been released yet; we'll need to do that before PheWeb can be released! I've been testing with local static files, representing the tip of the LZ.js develop branch

@abought
Copy link
Member Author

abought commented Sep 25, 2018

I've added back the field extraction mechanism. It does impose a namespacing restriction- this is still a change to PheWeb core behavior, but it makes it easier to use the same data source with all the other layers that already work that way.

Per the tooltip issue, I've traced this to pointer-events:none, the tooltip workaround. We're also seeing it with the exac link on genes tooltips in a modern pheweb. Will delegate that one to Peter as this was his workaround and he has more context.
http://pheweb.sph.umich.edu:5003/region/110.12/2:113925484-114325484

@abought
Copy link
Member Author

abought commented Sep 26, 2018

Also fixed the phewas scatter plot tooltips. @pjvandehaar , when would be a good time to meet and parcel out the remaining tasks needed to move this forward?

@abecasis
Copy link

abecasis commented Sep 26, 2018 via email

@abought
Copy link
Member Author

abought commented Oct 4, 2018

No word from Peter as yet. In the interests of meeting ASHG deadlines, and recognizing that it's already been ~10 days, I'm going to put go ahead and make most of the layout and behavior changes originally marked "for discussion", plus a few other tweaks.

Some of the things I held off on could introduce possible regressions, but we can discuss the details after the fact. Will update more tomorrow.

@abought
Copy link
Member Author

abought commented Oct 5, 2018

Tooltip functionality changed to support more granular hyperlinks (not just "click on dot to go to phewas").

Still need to identify test data with SNPs in the GWAS catalog; Peter would be more efficient at adding this. This concludes my current queue of work for PheWeb integration until we've looked over the direction and design plan.

screen shot 2018-10-05 at 12 38 04 am

@sgagliano
Copy link
Collaborator

Thanks Andy for all your work on this feature.
Peter, when are you able to address this PR?

@abought abought changed the title [WIP] Add GWAS catalog to PheWeb Add GWAS catalog to PheWeb Oct 6, 2018
Use a config variable to inject version into templates.

Ideally we would use a true package management/asset
build tool instead; this is admittedly a bit of a hack.
Represents bare minimum changes to restore core
functionality.
This removes the "get all assoc fields" functionality,
 pending discussion/ updates to use the new data chain refactor.
 The hack bypassed source namespacing (which hampered reuse of
 existing layouts). Primary use case seems to be arbitrary
 tooltips; req't needs clarification.

[ci skip]
with namespacing support

Also clean up some window.debug globals
(see breakpoint debugging as alternative)
- Make tooltip links clickable (remove pointer-events hack)
- Add "Make LD ref var" link back to tooltips
- Add "go to phewas" link to tooltips (since
  clicking directly does not do the trick)

- Remove keys found in base layouts
- DRY LD source via super call
- Remove custom padding style, as this was leading to
  display artifacts
(TODO: Improve synthetic data so we can test this)

[ci skip]
@abought
Copy link
Member Author

abought commented Oct 15, 2018

I've pushed (most of) the changes from discussion branch 8cdb7...pull-112-head#diff-007e08bf75bfe53b85b14034803e369c . These are intended as a companion to statgen/locuszoom#150.

I'll merge the LZ.js PR tonight barring any further requests.

@abought
Copy link
Member Author

abought commented Oct 15, 2018

A note: tests are passing locally (after incorporating the test_all fix- thanks!)

They are failing on travis even after restarting the build, but the error message is opaque. Peter, have you seen that failure log before?

@pjvandehaar
Copy link
Collaborator

I’ll try a different gencode url later.

@abought
Copy link
Member Author

abought commented Oct 15, 2018

Gencode url? I might be missing context here- what sort of different data do you need?

(Would any of the pending LZ data source changes be of use?)

@abought
Copy link
Member Author

abought commented Jan 14, 2019

LocusZoom 0.9 was officially released in late December, containing several of the items requested/used by PheWeb.

What is the status of this PR as a possible candidate for merge? Does it need updating?

Given the historical timeline, I'll hold off on making revisions until I hear back, but help is available on that basis.

@pjvandehaar
Copy link
Collaborator

This is now on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants