-
Notifications
You must be signed in to change notification settings - Fork 174
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
Added SVCLAIM field #517
Added SVCLAIM field #517
Conversation
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.
@d-cameron That's great! I like this explicit specification of a claim type very much. I added a comment and a suggestion about expanding the description of this somewhat. Also, there's a merge conflict with master branch now.
How do you think this PR should relate to #465? Will you update that one to reflect this change in approach?
I also wanted to comment on this:
I think that, once the comments are addressed, we can go ahead and merge this PR, and the example files can be added in a later PR. It helps to try to reduce the number of PRs opened simultaneoulsy. |
|
Co-authored-by: jmmut <jomutlo@gmail.com> Co-authored-by: Kirill Tsukanov <tskir@users.noreply.github.com>
Reformatted to one sentence per line Added paragraph on the the multiple interpretations of duplication calls Changed to Number=A. Made interpretation explicit of missing values explicit
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.
That's great! I really like where we're headed with this. Probably just one more iteration of edits will do it.
BTW, it's still showing a merge conflict. Not sure where it's coming from though, since the change only affects this one isolated section. Could you please verify manually that there are no conflicts?
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.
I already reviewed this in the past, so only minor changes remaining from my point of view.
And also could you please address three comments by @ctsa?
Changed PDFs as of fb7cb12: VCFv4.4.draft (diff). |
Changed PDFs as of 093e4ee: VCFv4.4.draft (diff). |
Changed PDFs as of 21cf1fa: VCFv4.4.draft (diff). |
Would like to add examples of how a CNV and SV caller would report transposition events (both replicative and conservative), as well some more complicated ones such as CN-neutral chromothripsis.
Waiting on #491 for actual files.
Should we be putting more examples directly into the VCF document itself? I'm of the opinion that the specs themselves should be clean, but come with a relatively extensive set of example stand and edge case VCFs.