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

SW implementation is command line argument in FilterAlignmentArtifacts #7105

Merged
merged 4 commits into from
Oct 26, 2021

Conversation

davidbenjamin
Copy link
Contributor

Closes #7087. @fleharty can you review this?

@droazen
Copy link
Collaborator

droazen commented Mar 22, 2021

@fleharty Any chance we could get a quick review on this one so that we can close out #7087 ? Thanks!

@davidbenjamin
Copy link
Contributor Author

Closes #7162.

Copy link
Contributor

@meganshand meganshand left a comment

Choose a reason for hiding this comment

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

Would be good to have the doc for the arg match the default, but otherwise 👍

@@ -135,10 +131,15 @@
@Argument(fullName= AssemblyBasedCallerArgumentCollection.BAM_OUTPUT_LONG_NAME, shortName= AssemblyBasedCallerArgumentCollection.BAM_OUTPUT_SHORT_NAME, doc="File to which assembled haplotypes should be written", optional = true)
public String bamOutputPath = null;

@Advanced
@Argument(fullName = AssemblyBasedCallerArgumentCollection.SMITH_WATERMAN_LONG_NAME, doc = "Which Smith-Waterman implementation to use, generally FASTEST_AVAILABLE is the right choice", optional = true)
public SmithWatermanAligner.Implementation smithWatermanImplementation = SmithWatermanAligner.Implementation.JAVA;
Copy link
Contributor

Choose a reason for hiding this comment

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

You want the default here to be JAVA, not FASTEST_AVAILABLE? If so, might be helpful to update the doc for this arg to say that JAVA is the preferred implementation. (And possibly leave a comment somewhere to the issue causing you to choose the JAVA implementation if that exists somewhere.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched it to fastest available.

Copy link
Contributor

@fleharty fleharty left a comment

Choose a reason for hiding this comment

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

Two trivial comments. Looks good to me otherwise.

import org.broadinstitute.barclay.argparser.ArgumentCollection;
import org.broadinstitute.barclay.argparser.CommandLineProgramProperties;
import org.broadinstitute.barclay.argparser.ExperimentalFeature;
import org.broadinstitute.barclay.argparser.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use explicit imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -135,10 +131,15 @@
@Argument(fullName= AssemblyBasedCallerArgumentCollection.BAM_OUTPUT_LONG_NAME, shortName= AssemblyBasedCallerArgumentCollection.BAM_OUTPUT_SHORT_NAME, doc="File to which assembled haplotypes should be written", optional = true)
public String bamOutputPath = null;

@Advanced
@Argument(fullName = AssemblyBasedCallerArgumentCollection.SMITH_WATERMAN_LONG_NAME, doc = "Which Smith-Waterman implementation to use, generally FASTEST_AVAILABLE is the right choice", optional = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the word "Which" is unnecessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@fleharty
Copy link
Contributor

@davidbenjamin Sorry, I reviewed this sometime ago, but didn't push the button to submit the review :(

@fleharty
Copy link
Contributor

@davidbenjamin Oops, I shouldn't approve changes that break the tests.

@davidbenjamin
Copy link
Contributor Author

The broken test seems to be accidental removed the import for @Advanced. Hopefully they will pass now.

@davidbenjamin davidbenjamin merged commit f74cf29 into master Oct 26, 2021
@davidbenjamin davidbenjamin deleted the db_choose_sw branch October 26, 2021 20:26
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.

FilterAlignmentArtifacts should allow the Java SmithWaterman implementation to be selected via an argument
4 participants