Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
bowtie2: remove code duplication #1645
bowtie2: remove code duplication #1645
Changes from all commits
3ae8cbf
dcffd93
0d00452
9dd083f
94e8c3a
ff4b297
b456d8f
971a889
9097abf
bcd7fe5
c8d3de2
4e60339
dcf9c07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have been all deleted because the BAM won't be present without the index files? Is there any harm in leaving them in as a sanity check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was auto-generated by
create-test-yaml
, so I've no vested interest in the contentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have no idea why the index files have ever been included in the output from
bowtie2 align
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no idea what happened here 😏 Does
/output/bowtie2/bowtie2/
exist when you runpytest
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, in the output directory not the work directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. So the indices are published so why are they being removed from the
test.yml
?🤔 This scares me a little just in case there are instances wherecreate-test-yml
is missing files it shouldn't. Worth checking again before the next release.Would you mind creating another issue please with some commands to reproduce?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one for you @chris-cheshire. You added these entries to
test.yml
for large indices but we don't have the corresponding entries inmain.nf
to actually test them? This is why all of these lines have automatically been deleted bycreate-test-yml
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh do we need to though? they use the same entry point, they just change a parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the
workflow
inmain.nf
needs to be explicitly duplicated for each test case. Maybe you can add these in a follow up PR after this is merged? Can ping me to review.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah my bad will do