-
Notifications
You must be signed in to change notification settings - Fork 596
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
Propagated all hidden arguments in gCNV CASE mode from the given model #7464
Conversation
…d CopyNumberCallingConfig arguments from the input model configuration.
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.
Let's add a documentation and checks on the Java side for when users try to specify hidden parameters with this:
final CommandLineArgumentParser clpParser = (CommandLineArgumentParser) getCommandLineParser();
final boolean isOutputSet = clpParser.getNamedArgumentDefinitionByAlias(StandardArgumentDefinitions.OUTPUT_LONG_NAME).getHasBeenSet();
and throw an error.
* then specified by a file contained in the model directory, all interval-related arguments are ignored in this | ||
* mode, and all model intervals must be present in all of the input count files. The tool output in CASE mode | ||
* is only the "-calls" subdirectory and is organized similarly to that in COHORT mode.</p> | ||
* then specified by a file contained in the model directory,and all model intervals must be present in all of the |
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.
directory,and
-> directory, and
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.
Done
* mode, and all model intervals must be present in all of the input count files. The tool output in CASE mode | ||
* is only the "-calls" subdirectory and is organized similarly to that in COHORT mode.</p> | ||
* then specified by a file contained in the model directory,and all model intervals must be present in all of the | ||
* input count files. All interval-related arguments (e.g. {@code interval-psi-scale} argument) are ignored in this |
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.
(e.g. {@code interval-psi-scale} argument)
-> (e.g. {@code interval-psi-scale})
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.
Done
* is only the "-calls" subdirectory and is organized similarly to that in COHORT mode.</p> | ||
* then specified by a file contained in the model directory,and all model intervals must be present in all of the | ||
* input count files. All interval-related arguments (e.g. {@code interval-psi-scale} argument) are ignored in this | ||
* mode, however an advanced user can adjust various sample-related (e.g. {@code sample-psi-scale}) and global |
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.
mode, however
-> mode. However,
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.
Done
* input count files. All interval-related arguments (e.g. {@code interval-psi-scale} argument) are ignored in this | ||
* mode, however an advanced user can adjust various sample-related (e.g. {@code sample-psi-scale}) and global | ||
* (e.g. {@code p_alt}) arguments for custom applications of the tool. Inference-related (e.g. | ||
* {@code min_training_epochs}) arguments can be adjusted as well. The tool output in CASE mode is only the "-calls" |
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.
Inference-related (e.g. {@code min_training_epochs}) arguments
-> Inference-related arguments (e.g. {@code min_training_epochs})
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.
Done
Random request, but maybe let's add a link to the white paper at https://github.com/broadinstitute/gatk/blob/master/docs/CNV/germline-cnv-caller-model.pdf while you're touching up the tool docs? |
d80d049
to
08f38d9
Compare
@samuelklee I will add that to the documentation as well. |
17bc5bf
to
ca5c0ee
Compare
@mwalker174 Thank you for the comments! Back to you |
…if any arguments not applicable in CASE mode were set by the user.
ca5c0ee
to
c8b5266
Compare
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.
Thanks - looks great!
gCNV in the CASE mode now fills in all hidden DenoisingModelConfig and CopyNumberCallingConfig arguments from the input model configuration.
This addresses issue #6994