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

problem with chromosome mapping in vcf #549

Merged
merged 2 commits into from
May 7, 2021

Conversation

piotr-gawron
Copy link
Contributor

@piotr-gawron piotr-gawron commented May 6, 2021

This PR address small issue with mapping chromosomes ids to numbers. When vcf file contained chromosomes like chr20 the id was mapped to 0 instead of 20.

There is also a unit test showing the issue.

@coveralls
Copy link

coveralls commented May 6, 2021

Coverage Status

Coverage increased (+0.05%) to 91.352% when pulling d7f4321 on piotr-gawron:chromosome-mapping-issue into 39ca07c on hammerlab:master.

Copy link
Collaborator

@akmorrow13 akmorrow13 left a comment

Choose a reason for hiding this comment

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

Great catch @piotr-gawron! One small question, other then that looks great! Thank you for your contribution.

@@ -152,8 +152,8 @@ class ImmediateVcfFile {

var contigMap = {};
contigs.forEach(contig => {
if (contig.slice(0, 3) == 'chr') {
contigMap[contig.slice(4)] = contig;
if (contig.slice(0, 3) === 'chr') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change to strict equality here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it probably out of habit. I never use == in JavaScript. There are too many pitfalls. My IDE also complains about == most of the time.

Probably in this case it does not matter so please revert. I did not want to interfere with your code style.


Another thing. I'm currently working on two features for pileup:

  • support for tabix index in vcf file. So you would be able to visualize huge vcf files without network performance issue. There is a javascript library that deals with tabix indexes (https://github.com/GMOD/tabix-js). I'm just in a process of making it work in browserify (there are some dependencies that must be fixed).
  • support for custom track visualization - in a project I'm working on there is need for visualization of project-related data. I can fork from pileup and modify the code (in fact I already have it working). However, I was thinking about making it more generic, so user would be able to plug in to canvas drawing of his own track.

I hope the functionalities could be introduced in the main pileup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great @piotr-gawron! Please let me know if there is anything I can do to help with these features.

I am curious, what custom track visualizations have you built? Do you think any of them would be useful for bringing into pileup.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a lot of variant data and the idea is to allow developer to implement filtering and custom visualization of the data together with crosslinks to annotations. I'm not even sure what the biologist would require from me at this point, I have only general overview of the requirements.

This is very had to generalize because every researcher is looking at the data differently and not everything is standardized. Therefore I wanted to open the possibility to customize it and not be forced to modify pileup with every tiny change request.


I will probably create a PR with tabix support as soon as the following pull requests will be pushed into npm:

I already have it working locally so it's just a matter of cleaning the code and fixing dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! Very excited to see this @piotr-gawron!

moved === to ==
@akmorrow13 akmorrow13 merged commit 88290d0 into hammerlab:master May 7, 2021
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.

None yet

3 participants