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

Add refinery max iterations arg #103

Merged
merged 8 commits into from
Jul 28, 2023

Conversation

AroneyS
Copy link
Collaborator

@AroneyS AroneyS commented Jul 28, 2023

Can be set to 0 to skip refinery (though it does still run CheckM)

@AroneyS AroneyS requested review from rhysnewell and wwood July 28, 2023 02:53
@rhysnewell
Copy link
Owner

Looks good, though is the confirmation that refining gets skipped when the parameter is set to 0 just from personal tests? The refining script didn't need any changes for that to work?

@AroneyS
Copy link
Collaborator Author

AroneyS commented Jul 28, 2023

Looks good, though is the confirmation that refining gets skipped when the parameter is set to 0 just from personal tests? The refining script didn't need any changes for that to work?

Yeh, just from testing. The script uses a while loop that wont run if =0. Though if we want to skip refinery we should really skip the CheckM rules too...

Copy link
Owner

@rhysnewell rhysnewell left a comment

Choose a reason for hiding this comment

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

I would prefer if checkm remains to be run by default so users get both checkm1/2 results so happy with how this runs now, users could opt in to skip it at some point. But we'd also need to add in a way to calculate the MAG stats that checkm1 does, wouldn't be too hard but just extra busy work.

@AroneyS
Copy link
Collaborator Author

AroneyS commented Jul 28, 2023

Oh definitely, I just meant the metabat2_checkm, semibin_checkm and rosella_checkm

@rhysnewell
Copy link
Owner

Ah gotcha, yeah they should get skipped for sure but this is good for now. Maybe a way to feed the iteration parameter into those checkm rules, if it's zero then just generate the expected file and exit. I believe there is already a similar check in the rule to make sure bins actually exist in the folders, maybe can use that

@AroneyS
Copy link
Collaborator Author

AroneyS commented Jul 28, 2023

Ah gotcha, yeah they should get skipped for sure but this is good for now. Maybe a way to feed the iteration parameter into those checkm rules, if it's zero then just generate the expected file and exit. I believe there is already a similar check in the rule to make sure bins actually exist in the folders, maybe can use that

Good idea. I'll add that here. Also need to add the bin check to Metabat2 and Semibin. I have had Metabat2 return no bins when other bins do.

@AroneyS AroneyS marked this pull request as draft July 28, 2023 03:35
@AroneyS AroneyS marked this pull request as ready for review July 28, 2023 04:10
@AroneyS AroneyS requested a review from rhysnewell July 28, 2023 04:10
@AroneyS
Copy link
Collaborator Author

AroneyS commented Jul 28, 2023

I moved the logic to python script. Should skip checkm_metabat/semibin/rosella when refinery iterations is 0 or when there are no bins detected

Copy link
Owner

@rhysnewell rhysnewell left a comment

Choose a reason for hiding this comment

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

Looks good, just add an alternative debug message to delineate between the case when no bins were found for a binner and max refinery iterations was set to 0

aviary/modules/binning/scripts/run_checkm.py Outdated Show resolved Hide resolved
@rhysnewell
Copy link
Owner

All good now, ready to merge?

@AroneyS
Copy link
Collaborator Author

AroneyS commented Jul 28, 2023

How do you sign commits?

@rhysnewell
Copy link
Owner

https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

My understanding was that you have to be using ssh rather than https when you push and pull from the repo, might not be possible with the qut system now that I think about it. I'll revert that requirement for pull requests, just means that the commits that you send through won't be verified by github and you won't get credit for them on your homepage

@AroneyS AroneyS merged commit 1a699ac into rhysnewell:dev Jul 28, 2023
1 check passed
@AroneyS AroneyS deleted the add-refinery-max-iterations-arg branch July 28, 2023 04:38
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