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

Update PheWeb for changes in LZjs 0.14.0 #185

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abought
Copy link
Member

@abought abought commented Dec 1, 2021

🦑 Hold for final release of LZ 0.14.0

Purpose

The next release of LocusZoom makes breaking changes to how data is retrieved. This PR updates PheWeb to work with the new version, and introduces small user-facing benefits in terms of larger font sizes and a more concise legend for association plots. This will help people to use figures from PheWeb in publications or presentations.

Blockers

Hold for the final release of LocusZoom.js 14, so that we can update the version string in this PR before merge. (I'm hoping that will happen soon)

Testing notes

  • The original data retrieval code was.... verbose. Let me know if I missed any edge cases
  • I modified the API to send log_pvalue (instead of just pvalue). It seems silly to duplicate, and this could be fragile in cases of numerical underflow. Is there a pheweb branch that uses log pvalue internally?
    • Peter should check this section to make sure that very small pvalues don't trigger a python exception. I did not test thoroughly, because calculating log on the fly isn't my ideal solution (storing and sending things as log_pvalue seems more robust)

Simplifies the data retrieval code (zomg), and contains minor visual improvements for use of figures in publications: mostly larger font sizes and more concise legend
@pjvandehaar
Copy link
Collaborator

No, there's not a branch that uses logpvalue. The python needs to check for pval==0 and then it's fine.

What needs to happen to merge this? Update to 14.0 (no beta) and test?

@abought
Copy link
Member Author

abought commented Jan 2, 2023

What needs to happen to merge this? Update to 14.0 (no beta) and test?

Hi Peter!

In general the branch changes should work well, but I didn't exhaustively test every feature of PheWeb (and due to breaking LZ 14 layout syntax changes, it needs a little more attention to testing than I had bandwidth for back then).

At very least the branch should mostly function for what I did test: I don't think I made any major edits to LZ between December and when v14 was actually released (there was a long holding period due to external partners).

As for the pvalue/logpvalue issue: as long as PheWeb is happy with current situation, we're good. I do like storing things as logp where possible (esp for things like top hit detection, as modern gwas seem to have more extreme hits)..... but that architectural change is probably not in scope based on pheweb's current "all volunteer" maintenance situation.

(I've changed projects as well, and don't presently have the time to backport the top hit & qq revisions from my.lz.org, which uses the logp internally. Ah well.)

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.

2 participants