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

Use StandardArgumentDefinitions for arguments #133

Open
lbergelson opened this issue Jan 21, 2015 · 16 comments
Open

Use StandardArgumentDefinitions for arguments #133

lbergelson opened this issue Jan 21, 2015 · 16 comments

Comments

@lbergelson
Copy link
Member

We should ensure a standard naming scheme for input and output parameters.

requires #67

@lbergelson
Copy link
Member Author

Current Picard tools have boolean flags with the default set to true is this a thing we want to allow?

@vdauwera
Copy link
Contributor

I prefer the classic GATK paradigm, in which by default every boolean is false, until you pass the flag to set it to true. This allows you to be able to just add the flag to the CL without specifying a value. From user POV this provides valuable consistency. It seems a lot more intuitive as well. It's like you're asking "do you see a flag?" and in the answer, no means no, and yes means yes.

@akiezun
Copy link
Contributor

akiezun commented Jan 22, 2015

getopt_long which is the de-facto GNU standard, permits no-argument flags and so should we I think, http://linux.die.net/man/3/getopt_long (btw, @lbergelson @droazen I think the standard permits both space and equal sign as a separator, see "A long option may take a parameter, of the form --arg=param or --arg param.")

@akiezun
Copy link
Contributor

akiezun commented Apr 20, 2015

I think @yfarjoun had a usecase for --flagname true (it makes sense if you're generating the commandline and don't want to write special case code to handle boolean arguments). @vdauwera can you clarify and state the requirements here explicitly? Unassign when clarified.

@lbergelson
Copy link
Member Author

Currently parsing accepts boolean flags either as just a --flag for true,
or --flag true / --flag false.
On Apr 19, 2015 11:24 PM, "Adam Kiezun" [email protected] wrote:

I think @yfarjoun https://github.com/yfarjoun had a usecase for --flagname
true (it makes sense if you're generating the commandline and don't want
to write special case code to handle boolean arguments). @vdauwera
https://github.com/vdauwera can you clarify and state the requirements
here explicitly? Unassign when clarified.


Reply to this email directly or view it on GitHub
#133 (comment)
.

@akiezun
Copy link
Contributor

akiezun commented Apr 20, 2015

so is there anything to do here?

@lbergelson
Copy link
Member Author

Yes, lots of stuff to standardize still. We need to do a pass over all of the tools and make sure arguments like input bam are always named the same way.

@akiezun
Copy link
Contributor

akiezun commented Apr 20, 2015

ok, that is definitely a UserDoc ticket. @vdauwera it's your call on what needs to be done

@vdauwera
Copy link
Contributor

Is there a way we can use constants for common arg names? Would be the simplest way to keep them all in sync IMO.

@yfarjoun
Copy link
Contributor

@lbergelson, @akiezun AFAIK picard tools need you to specify specifically FLAG=true/ FLAG=false if it's a boolean flag. it is true that, if you want, any argument can have a default value (true or false) but to change it you will still need to assign true or false (i.e even if there is a default you cannot simply have FLAG on the commandline)

Yes, the logic of the pipeline specifies all the commandline arguments, regardless of defaults so that if the defaults change (which the GATK used to do all the time!) the pipeline will not change. Thus the use case has to include being able to set all arguments to their value boolean or otherwise.

@lbergelson
Copy link
Member Author

@yfarjoun That's true for picard tools. In hellbender a flag without a default value is implicitly false, and a flag that has a default value of true is bad form and should be inverted.

I understand your use case and see the value of it, which is why you can already also specify --flag true and --flag false, or even -flag=false if you want.

@lbergelson
Copy link
Member Author

@vdauwera We can (and should) definitely use constants for names. A lot of them live in StandardArgumentDefinitions. Not all tools use these constants though, and I don't think we have broad agreement on the names/ value of each.

On a side note, StandardArgumentDefinitions is really long and looks awful to see over and over again. I'd recommend we standardize on statically importing this.

@vdauwera
Copy link
Contributor

Yay! Glad to hear this. I can do a pass (or get a secon to do a pass) through the tools to check for tools that don't follow this, and review/revise names where appropriate. Later we can enforce it in review.

@vdauwera
Copy link
Contributor

Could abbreviate to SAD ;)

@akiezun
Copy link
Contributor

akiezun commented Apr 20, 2015

@lbergelson so it seems that nothing more needs to be done for the flags, right? Both the GATK and the Picard syntaxes are supported. If that's true, let's rename the issue to "use StandardArgumentDefinitions for arguments" or something and clarify the requirements. If that's not true, then please explain what more needs to be done to support both GATK and Picard syntaxes.

About the name, how about ArgumentNames?

@lbergelson
Copy link
Member Author

I think the flags are done.

I like ArgumentNames. We should update our test code to use the constants rather than strings as well.

@lbergelson lbergelson changed the title Standardize input parameters Use StandardArgumentDefinitions for arguments Apr 20, 2015
@akiezun akiezun modified the milestone: Coelacanth Apr 20, 2015
lbergelson pushed a commit that referenced this issue May 31, 2017
… correctly.

Addressing issue #133

All unit tests pass, though some more need to be added.

Fixing ACS conversion

Fixed error with sample column being retained, when it is not needed.

More corrections to ACS output.

More corrections to ACS output.

Simple balanced segment caller.  Still needs automated testing.

Reverted ACNV refactoring.

Fixing compile errors

Refactoring for better code re-use of SegmentUtils.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants