Skip to content

Development of somalier_wrapper.py#1

Open
ASendellPrice wants to merge 23 commits into
mainfrom
development
Open

Development of somalier_wrapper.py#1
ASendellPrice wants to merge 23 commits into
mainfrom
development

Conversation

@ASendellPrice

Copy link
Copy Markdown
Contributor

somalier_wrapper.py is a wrapper script for streamlining the execution of somalier extract, relate, and ancestry commands into a single command. It provides a stand alone method of assessing sample relatedness outside of existing bioinformatics pipelines that can be used when addressing ad hoc queries regarding relationships between samples.

Comment thread README.md

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a really good readme, very comprehensive!

Comment thread bin/somalier_wrapper.py
"--fasta", help="Path to reference genome", required=True
)
argument_parser.add_argument("--sites", help="Path to sites file", required=True)
argument_parser.add_argument("--prefix", help="Prefix for results", required=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for any of these required arguments that'll be the same every time (e.g. sites file), we add a default so we don't need to give the arguments every time? or if you want it to be more general, we could put those arguments in a config to pass in. Not a necessity though, just a suggestion

Comment thread env/somalier_wraper.yaml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

could you add versions for the dependencies

Comment thread bin/somalier_wrapper.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

really good commenting throughout here, it's easy to pick this up and follow what you're doing

Comment thread bin/somalier_wrapper.py
)

# Gather command line arguments
args = parse_arguments()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we want to do an upfront check that if you're running with relatedness turned on that you've supplied all three arguments? Would save running something long for it to crash part way

@Istaisa

Istaisa commented Nov 8, 2024

Copy link
Copy Markdown

Initial review of logic, will check with test data post-move.

This is a really good script, clearly written and it's easy to follow what's going on.
Given the nature of the script focusing on running existing software via subprocess, I'm actually not going to suggest unit testing here, but it would be good longer term to have a tiny lightweight test dataset with known outputs to verify it's working as expected, but I know you've done a lot of work around this.

I agree with your suggestion that Nextflow would probably be a better approach for comparing many many samples, but this should work well in Python for comparing within a run or between a couple of runs. I think the best next step is working out some thresholds for each of the different assays so we can look at implementing this in the pipelines.

@ASendellPrice

Copy link
Copy Markdown
Contributor Author

Hi @Istaisa, thank you for code review. Your comments will be really helpful as I continue to develop somalier_wrapper.py. I will make the relevant changes over the next week 😄

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.

2 participants