-
Notifications
You must be signed in to change notification settings - Fork 16
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
Treenome Browser v1 integration #302
Conversation
Merge upstream
Ok, latest commit should fix that horizontal zoom issue! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for all the hard work!
Oh yeah I remember this thing about the build failure to Docker. I looked into that a bit before: it's because of a canvas dependency from JBrowse. Will have a think about that.. |
What might work is to: set https://github.com/theosanderson/taxonium/blob/master/Dockerfile.frontend#L1 to |
It seems like it is due to the fact that alpine uses |
Ah good catch – definitely fine with that! |
I'm not depending on the Docker image at the moment -- that was in use on a version within GISAID but that project seems to have stalled -- so feel free to merge this without fixing that and I can try to figure it out in due course |
Looks like the image is building okay now. Another issue was that canvas doesn't have builds for Node 18 yet. As far as I can tell, in the releases, Let me know if you think downgrading to 16 is an issue. I don't know enough about node/docker to know why this was only an issue in the Docker build... |
Apologies that I got this wrong!
Hmm interesting. It definitely would be an issue for the backend, which uses the fetch API (which is normally in the browser, not node, but was added experimentally in node 17 and properly in node 18). Somehow I convinced myself it was needed generally but I think you're right that it's not.
Yeah me too :( |
Ok, I think this should do it! Went back to Node 18 alpine, and now canvas is built from source first before installing everything else. It does unfortunately take quite a bit longer to build now (~5m). BTW, should the integration tests be failing? I think it's due to a permissions issue (I had removed my write access so I wouldn't accidentally push to main again 😬 ) |
Yeah this is permissions but not to worry -- I think it will be fine, we can definitely merge as is. Are you happy for me to merge now? |
Awesome. I am ready if you are! |
OK, let's see how this goes! Thanks for the amazing feature and working through all these points. |
🎉 🎉 everything seems to be working, tests passing, etc. |
Yay! Thanks for all of your help and patience! |
Here's a PR where we can iron out the final details of the treenome browser.
A couple recent additions:
I think the last main thing to figure out is, as you mentioned, handling non-SARS-CoV-2 trees.
Right now it really only works for the cov2tree tree. The two main things needed to build the browser for an arbitrary tree are (1) the reference sequence and (2) coordinates of genes/CDS to compute nt positions of aa mutations. (2) is only necessary for aa mutations, so just displaying nt mutations could work without it.
I currently compute the reference through the fake "mutations" at the root node -- which I realize aren't guaranteed to be there (but I think they should be if a .gb file was used to make the tree)
(2) is hard-coded for SARS-CoV-2. But if we stored the CDS data from a .gb file, I could use those.
What do you think about storing the genbank CDS features (potentially also the fasta sequences) in the jsonl file, and only displaying the treenome toggle button if they are present?