Ultima develop code merge

[dror27]

This pull request includes Ultima Genomics code contributed to the public IGV version.

Code mainly has to do with additional rendering features for Flow Based tracks, specifically graphic indications of INDEL qualities.

NOTE: please hold on reviewing this pull request, pending out-of-bound communication,.

[ilyasoifer]

@jrobinso - this is the PR related to #1393. Can you suggest how to move forward with that? I am also attaching the documentation of the changes:
IGV Supplemental.pdf

[jrobinso]

@ilyasoifer Apologies for the delay, I am on vacation returning Nov 19. Will respond further via email.

[jrobinso]

WRT to next steps, we will need a test case we can explore ourselves. Also, eventually we would request the documentation in Markdown, as that is the language used for our documentation system (https://igv.org/doc/desktop/). Thanks.

[ilyasoifer]

@jrobinso - thanks for the feedback. We will add the test data (I assume to test/data). Could you please point me to the repository that contains the documentation so I can see how the md documents you are using are structured? Enjoy your vacation!

[dror27]

@jrobinso - thanks for the feedback. We will add the test data (I assume to test/data). Could you please point me to the repository that contains the documentation so I can see how the md documents you are using are structured? Enjoy your vacation!

flow_renderer.sam added to test/data/sam. Once @ilyasoifer adds doc we can point to specific reads & locations for each new feature. Data is around chr1:10000

[helgathorv]

@ilyasoifer Our new doc repo is not yet public, but here are some sample markdown files from the User Guide > Tracks and Data Types > Alignments. Let me know if you have any questions.
SampleIGVDoc.zip

[helgathorv]

@ilyasoifer The IGV doc repo is now public at https://github.com/igvteam/igv-docs-website

[ilyasoifer]

Thanks @helgathorv for these examples. Should we create a PR for the documentation in that repository or attach it to this PR?

[jrobinso]

@ilyasoifer As this is just a single page of doc attaching it here is fine.

[ilyasoifer]

@jrobinso and @helgathorv I added to this PR a folder called doc that contains a markdown file igv.md. Since the document also contains images, I thought it is more convenient to add it this way. Let me know if you would like me to otherwise re-arrange.

[ilyasoifer]

@dror27 - I think our code contains a small bug similar to what we encountered in broadinstitute/gatk#8337.
Specifically, the code automatically recognizes data that contains tp tag as Ultima. However, minimap2 aligner apparently also uses tp tag for a different reason.
Could you implement something similar to what I did in that PR? Thanks!

[dror27]

@dror27 - I think our code contains a small bug similar to what we encountered in broadinstitute/gatk#8337. Specifically, the code automatically recognizes data that contains tp tag as Ultima. However, minimap2 aligner apparently also uses tp tag for a different reason. Could you implement something similar to what I did in that PR? Thanks!

Logic added to screen flow alignments based on read-group presence and fields

[jrobinso]

Hi all, apologies for the lack of response, I've been delayed by travel and some pressing issues but will get to it ASAP.

[jrobinso]

Hi all, I might have missed but have instructions for use been posted somewhere? The documentation describes the visual features but how do we actually try it?

BTW the doc pages have been added to the igv-docs-website, any further changes to docs should be done there. Currently the page is at https://github.com/igvteam/igv-docs-website/tree/main/igv-docs/docs/UserGuide/tracks/alignments/ultima.

I'm concerned about this new (to IGV) concept of "extensions". Its not part of our current design, obviously, and I am not sure we want to take on a new concept such as this for the sake of a few visiual enhancements for data from a specific vendor. It might be a good concept, that we would consider as part of a major refactor, but is it required for the enhancements we are considering here?

[dror27]

Hi all, I might have missed but have instructions for use been posted somewhere? The documentation describes the visual features but how do we actually try it?

BTW the doc pages have been added to the igv-docs-website, any further changes to docs should be done there. Currently the page is at https://github.com/igvteam/igv-docs-website/tree/main/igv-docs/docs/UserGuide/tracks/alignments/ultima.

I'm concerned about this new (to IGV) concept of "extensions". Its not part of our current design, obviously, and I am not sure we want to take on a new concept such as this for the sake of a few visiual enhancements for data from a specific vendor. It might be a good concept, that we would consider as part of a major refactor, but is it required for the enhancements we are considering here?

@jrobinso - as to the choice of implementing this visual feature using an extension modality - this has mainly been done as a the feature was developed over the past year or two - while IGV versions were moving on. The extension model allowed for isolating the code for the new visual feature into a separate set of classes, with minimal changes in IGV itself. This allowed updating the core IGV and tacking on it the visual extension.

This might not be required when the features are accepted as being part of the core IGV. Still, I can envision that some IGV users/developers might find it easier to use this extension mechanism to create new visualizations which would start as experimental and wind up in the general version.

The bottom line is that you - as the product's architects - need to take a decision. We will probably go with what ever you decide.

As to the doc, @ilyasoifer will respond.

[ilyasoifer]

@jrobinso Data had been placed here: test/data/sam/flow_renderer.sam. Data are around chr1:10000. I believe that our extension automatically detects the Ultima-specific tags and activates the modified visualization of the indels.

Thanks for moving the docs to the other repo, should we removed them from this PR?
Thanks!

Hi all, I might have missed but have instructions for use been posted somewhere? The documentation describes the visual features but how do we actually try it?

BTW the doc pages have been added to the igv-docs-website, any further changes to docs should be done there. Currently the page is at https://github.com/igvteam/igv-docs-website/tree/main/igv-docs/docs/UserGuide/tracks/alignments/ultima.

I'm concerned about this new (to IGV) concept of "extensions". Its not part of our current design, obviously, and I am not sure we want to take on a new concept such as this for the sake of a few visiual enhancements for data from a specific vendor. It might be a good concept, that we would consider as part of a major refactor, but is it required for the enhancements we are considering here?

[jrobinso]

@dror27 An extension framework might be a good idea in general, but we have then escalated this PR significantly. For one thing we would need documentation on the framework itself. Its an interesting idea but given constraints here it would be some time before we are able to consider it.

If I understand this PR the changes, from a user perspective, amount to some additional visual elements for indels and some additions to the popup text. I would not expect this to require many changes to code, I think the bulk of changes here are for the extension framework. If you could remove that (the extension) it would certainly expedite review. The "ext" package might be renamed to "ultima" for specialized code that remains, similar to the "smrt" (pacbio) package.

@ilyasoifer The doc can be removed from the PR, no requirement to do so, but if you wish to make changes let's do it with PRs to the doc website.

[dror27]

@jrobinso ok. I will update.

[dror27]

@jrobinso @ilyasoifer - I have now removed the framework (concept and code). Additionally, the package has been renamed to .ultima. Thank you for your input and review

[jrobinso]

@dror27 Thanks for the quick turnaround, much easier to review now. I had left some comments on particular files prior to this, hopefully you can see them. I have not used the github review function before. 2 in particular, could you explain the changes to SessionWriter and LoadFromURLAction? I don't understand the SessionWriter change relevance, and am not sure about the "multiple tracks" addition either. LoadFromURLAction already supports loading multiple urls, what does this change allow?

[dror27]

@jrobinso I was not able to see your individual comments, but I understand your concerns.

The two changes mentioned (loading mutliple tracks and session-writer changes) were remains of behaviors of previous IGV versions that we patched over. They are no longer required and were therefore removed.

[jrobinso]

This is a new method, most of it moved from somewhere else I think. The github diff tool here makes it hard to see what has actually changed, and what the consequence of this new method will be, but is it neccessary in the first place? Fewer changes are better.

[dror27]

@jrobinso this method, drawInsertions, if I understand correctly, was authored outside the scope of this pull request (see Git Blame information).

I did notice that it, along with the following methods in this class (VariantRenderer) is not used. You may consider removing them:

• drawInsertions
• renderExpandedInsertion
• getOutlierStatus
• compareToBounds

Running "unused declarations" code analysis produces many more unused methods - so I think that this is a bigger issue and might not be related to this pull request.

[jrobinso]

Thanks, could you remove this method? That will make for a cleaner merge. Also could you remove the doc as that has been moved to the igv doc repository? With this I think this will be ready to merge. Thanks for your prompt responses.

[dror27]

Done - method drawInsertions and superfluous doc removed.

[jrobinso]

OK great! I will do a pull and some quick tests and then merge, probably this evening. Thanks for you quick responses, it makes this easier.

[jrobinso]

Thanks again for your contributions. This is merged and will be part of the upcoming 3.0 release. We hope to publish a preview before the end of the year, with a final release early in 2024. If you would like to suggest a short description (1-2 sentences) for the release notes please feel free to do so here. Otherwise we will derive a description from the documentation.

[dror27]

@jrobinso thank you for the joint work and for the merge.

@ilyasoifer can you please suggest a short description (1-2 sentences) for the release notes? (we've removed the doc from the source tree).

My take on summarizing the deleted doc:

• New graphical annotations for INDEL quality on SAM/BAM/CRAM files containing Flow Based reads. For information on Flow Based sequencing, please see https://www.biorxiv.org/content/10.1101/2022.05.29.493900

[ilyasoifer]

@jrobinso - thanks for the efficient merge. I agree with @dror27 description for the release note, just one minor tweak:

• New graphical annotations for INDEL quality on SAM/BAM/CRAM files containing Ultima Genomics flow-based reads. For more information please see https://www.biorxiv.org/content/10.1101/2022.05.29.493900

[jrobinso]

@dror27 A problem has arose. We need a better test for "isFlow". The problem is "ti" and "tp" are not reserved tags, anyone can use these for any application and we now have non-Ultima data that uses "tp". So we need something more precise. Is there anything in the BAM header that would tell us this is a "flow" tagged file? Any other suggestions?

Problem is reported here (or rather symptom): #1468

  private boolean isFlow(SAMRecord record) {

        // must have one of the key attributes
        for ( String name : KEY_ATTR.split(",") ) {
            if ( record.hasAttribute(name) ) {
                return true;
            }
        }
        return false;
    }

[ilyasoifer]

Thanks, we will take care of this, hopefully today. Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Jim Robinson ***@***.***> Sent: Tuesday, January 23, 2024 6:11:41 AM To: igvteam/igv ***@***.***> Cc: Ilya Soifer ***@***.***>; Mention ***@***.***> Subject: Re: [igvteam/igv] Ultima develop code merge (PR #1423) @dror27<https://github.com/dror27> A problem has arose. We need a better test for "isFlow". The problem is "ti" and "tp" are not reserved tags, anyone can use these for any application and we now have non-Ultima data that uses "tp". So we need something more precise. Is there anything in the BAM header that would tell us this is a "flow" tagged file? Any other suggestions? Problem is reported here (or rather symptom): #1468<#1468> private boolean isFlow(SAMRecord record) { // must have one of the key attributes for ( String name : KEY_ATTR.split(",") ) { if ( record.hasAttribute(name) ) { return true; } } return false; } — Reply to this email directly, view it on GitHub<#1423 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGDPRCOLCD3YNQZBLJ3IBLLYP4Z73AVCNFSM6AAAAAA6UFUU4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBVGI3DAOJUHA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

________________________________ CONFIDENTIALITY NOTICE: This message (including any attachments) should be presumed to contain confidential, proprietary, privileged and/or private information. Information contained in this message is intended only for the recipient(s) named above. Any disclosure, reproduction, distribution or other use of this message or any attachments by any unauthorized individual or entity is strictly prohibited. If you have received this message in error, please notify the sender immediately, and delete the message and any attachments. Learn more about Ultima Genomics’ Privacy Policy<https://www.ultimagenomics.com/privacy-policy>.

[jrobinso]

@ilyasoifer Thanks, I have some ideas too but I'll wait for your suggestions. If you branch its probably better to do so from branch "2.17", as opposed to main, but either will do. It will be merged to both.

[dror27]

In newer IGV code, we also test for the "ULTIMA" value in the "PL" (platform) attribute of the associated read group, the presence for the "MC" (maxHmer) in the read group and finally the presence "tp" attribute on the record itself.

Does it make sense?

   final SAMReadGroupRecord readGroup = samAlignment.getRecord().getReadGroup();
    if ( !RG_ATTR_PL_ULTIMA.equals(readGroup.getAttribute(RG_ATTR_PL))
         &&  (readGroup.getAttribute(RG_ATTR_MC) == null) )
        return false;
    if ( !samAlignment.getRecord().hasAttribute(ATTR_TP)  )
        return false;

[dror27]

@jrobinso and @ilyasoifer - please see new pull request to address:
#1471

[jrobinso]

That looks pretty robust. I'm also going to add a try/catch for any future conflicts, but with this test I doubt they'll be any.