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 empty bin_info.tsv with --skip-abundances #127

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Conversation

AroneyS
Copy link
Collaborator

@AroneyS AroneyS commented Aug 28, 2023

My bad. Inner join removes all the other bin info...

@AroneyS AroneyS requested a review from rhysnewell August 28, 2023 02:58
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like you need to update the second merge as well. For safety, it should likely be a left join as well to keep all of the checkm output, also should change the column it is joining on to on=[merged_out.columns[0]] right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just in case taxonomy is missing a genome?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, I know there are cases where GTDB-tk will randomly fail to assign taxonomy to genomes due to some pplacer issue. Not sure what happens to the gtdb-tk output in that case, like it might just end up missing from the file. Additionally, there probably exists a world where running gtdb-tk ends up being an optional step so making it a left join future proofs against that I suppose

@AroneyS
Copy link
Collaborator Author

AroneyS commented Aug 28, 2023

Manual test running now

@AroneyS
Copy link
Collaborator Author

AroneyS commented Aug 28, 2023

Manual test succeeded

@AroneyS AroneyS merged commit 83a2518 into main Aug 28, 2023
@AroneyS AroneyS deleted the fix-skip-abundances branch August 28, 2023 04:45
@AroneyS
Copy link
Collaborator Author

AroneyS commented Aug 28, 2023

Noooooo

@AroneyS
Copy link
Collaborator Author

AroneyS commented Aug 28, 2023

Into main again :(

@rhysnewell
Copy link
Owner

haha it's okay, just catch dev up to main

@AroneyS AroneyS mentioned this pull request Sep 13, 2023
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