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

fix: classify mature mirnas #146

Open
wants to merge 44 commits into
base: dev
Choose a base branch
from

Conversation

deliaBlue
Copy link
Collaborator

This PR closes #143 .

The isomiR notation used until now was not unambiguous and lead to incorrect counts provided that the CIGAR and MD strings could be the same for different isomiR sequences. To account for this fact, the read sequence is added to the isomiR name.

The changes required to accomplish that are:

  • Modify the iso_name_tagging.py script to add the read sequence on the isomiR name
  • Modify the unit test for the script iso_name_tagging.py to account for the isomiR new name format, and the corresponding files. Given that the previous unit test file was not testing for the functions alone, those tests have been added.
  • Modify the mirna_quantification.py script to account for the new name format.
  • Modify the unit tests for the script mirna_quantification.py to account for the new name format, and the corresponding files.
  • Update the expected output of the pipeline to include the read sequence on the isomiR names

deliaBlue and others added 30 commits December 22, 2023 18:45
…olanlab/mirflowz into 126-docs-describe-workflow-rationale
@deliaBlue deliaBlue added bug Something isn't working high priority labels Aug 18, 2024
@deliaBlue deliaBlue requested a review from uniqueg August 18, 2024 17:29
@deliaBlue deliaBlue self-assigned this Aug 18, 2024
@deliaBlue deliaBlue linked an issue Aug 18, 2024 that may be closed by this pull request
@uniqueg uniqueg changed the title fix: classify correctly mature mirna fix: classify mature mirnas Aug 19, 2024
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

In some places, the documentation of the script is written as if the script is a general purpose script for adding intersecting features to SAM files as a tag. But in other places, it becomes clear that there are several assumptions that limit the scope of the script (e.g., miRNA_ID, shifts/extensions) to miRNAs and isomiRs. I think you could clarify that a bit better and perhaps also make a reference to the scripts that produce valid inputs to this file, because it is highly unlikely that someone would create the inputs for this script manually.

Btw, it would have actually been nice to design this script such that it actually is a general purpose script for adding "name" tags for intersecting features to SAM files and do all the other stuff (dealing with alignments that don't have an intersecting feature, dealing with maximum extensions etc) elsewhere. As it is, this script is quite complicated and has basically zero chance of reuse outside of this workflow.

Anyway, not important now - just lessons for the future :)

So please just clarify the scope of the script in the module-level docstring and I think we are ready to go.

scripts/iso_name_tagging.py Show resolved Hide resolved
scripts/iso_name_tagging.py Show resolved Hide resolved
@deliaBlue
Copy link
Collaborator Author

In some places, the documentation of the script is written as if the script is a general purpose script for adding intersecting features to SAM files as a tag. But in other places, it becomes clear that there are several assumptions that limit the scope of the script (e.g., miRNA_ID, shifts/extensions) to miRNAs and isomiRs. I think you could clarify that a bit better and perhaps also make a reference to the scripts that produce valid inputs to this file, because it is highly unlikely that someone would create the inputs for this script manually.

Btw, it would have actually been nice to design this script such that it actually is a general purpose script for adding "name" tags for intersecting features to SAM files and do all the other stuff (dealing with alignments that don't have an intersecting feature, dealing with maximum extensions etc) elsewhere. As it is, this script is quite complicated and has basically zero chance of reuse outside of this workflow.

Anyway, not important now - just lessons for the future :)

So please just clarify the scope of the script in the module-level docstring and I think we are ready to go.

I believe the script per se is pretty general: it adds a custom tag showing which features an alignment intersects with. I think the problem comes when defining variables and how it is documented. In this sense, the script appears to be only for miRNAs whose annotations might or might not been previously extended. But if the word extension is changed to shift or range and the description changes to "Allowed shift range between either end of the alignment and the intersecting feature" (or maybe a better description but just for you to get the idea) then it gets more general and the script does not have to change that much.
Another example would be the way the tag format is specified. If instead of using miRNA_ID I use intersecting_feature any kind of sequence can be used as long as it has been intersected using Bedtools intersect.

I suggest to try and make the descriptions and names more general and if you do not see it clear, I will just revert the commit and document a more restricted scope.

@uniqueg
Copy link
Member

uniqueg commented Sep 2, 2024

Yes, you can do that if you like. The shift stuff is still quite specific, but I think it's best to keep it, so that we can come to an end on this soon and publish the workflow :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: classify correctly mature miRNA
2 participants