kfuku52 / amalgkit

RNA-seq data amalgamation for a large-scale evolutionary transcriptomics
BSD 3-Clause "New" or "Revised" License
7 stars 1 forks source link

simplify subparsers #14

Closed kfuku52 closed 1 year ago

kfuku52 commented 4 years ago

The arguments shared among subparsers can be simplified like this. We can specify parents= in aadd_parser. https://stackoverflow.com/questions/7498595/python-argparse-add-argument-to-multiple-subparsers

kfuku52 commented 3 years ago

yes/no options can be stored as bool using strtobool. Please check csubst for example.

kfuku52 commented 3 years ago

I introduced strtobool for --tissue_detect as an example. Please check and apply it for other options. https://github.com/kfuku52/amalgkit/commit/f85b762ebec67934fa9d698e5a20393a86d3c97e

Hego-CCTB commented 3 years ago

Just to add to this, I think we can simplify usage as well for arguments that use 'yes|no'. For example amalgkit getfastq --redo yes can be reduced to just amalgkit getfastq --redo, while amalgkit getfastq --redo no would just be amalgkit getfastq without the option.

kfuku52 commented 3 years ago

I'm not positive about that. Here's how I often end up...

  1. Didn't work: my_progrem -i infile --hard-to-remember-flag
  2. Didn't work either: my_progrem -i infile
  3. Found another flag is important: my_progrem -i infile --another-flag
  4. Went program website to remember what was the flag I tried first
  5. Worked well: my_progrem -i infile --another-flag --hard-to-remember-flag

If it's like --hard-to-remember-flag yes, we don't have to open a webpage or print a very long help message every time we tweak the parameters.

Hego-CCTB commented 3 years ago

So I don't 100% get how strtobool interacts with argparse. I assumed it would convert argument input yes to True and input no to False. So a flag --flag1 would be False if the default was no and there wasn't any additional input.

Assuming --flag1 no, in the code I could then ask:

     if args.flag1:
          print("true!")
     else:
          print("false!")

instead of

if args.flag1 == 'yes':
           ...

correct?

I switched the getfastq download flags to strtobool like this:

     pge.add_argument('--aws', metavar='yes|no', default='no', type=strtobool, required=False, action='store',
                 help='default=%(default)s: Download SRA files from Amazon Cloud (AWS), if available.')

But when getting args.aws inside getfastq, they are still stored as 'yes' and 'no`, rather than 'true' or 'false'. This is actually not true, they were stored as 1 or 0.

Hego-CCTB commented 3 years ago

Scratch that, the problem was somewhere else!

Hego-CCTB commented 1 year ago

I went through all of amalgkit to convert everything to strtobool where there are yes|no options. I also introduced parent parsers to eliminate repeated code.

I couldn't figure out a way to do this with a single parent parser holding all the shared options. For example: --out_dir is by every subcommand. --metadata is used by most subcommands. But putting both --out_dir and --metadata in the same parent parser, both options would show up for example for amalgkit metadata -h, which does have --out_dir but does not have --metadata.

So I put those options into different parent parsers parent_parser_out and parent_parser_meta. These can then be combined in any way for the specific subparsers, like so:

# Sub parser: integrate
pin = subparsers.add_parser('integrate', help='see `amalgkit proc -h`', parents=[parent_parser_out, parent_parser_meta, parent_parser_threads])

[...] subparser specific arguments

# Sub parser: sanity
psa = subparsers.add_parser('sanity', help='see `amalgkit sanity -h`', parents=[parent_parser_out, parent_parser_meta])

[...] subparser specific arguments

# Sub parser: curate
pcu = subparsers.add_parser('curate', help='see `amalgkit curate -h`', parents=[parent_parser_out, parent_parser_meta, parent_parser_batch])

[...] subparser specific arguments
kfuku52 commented 1 year ago

Sounds good!

Hego-CCTB commented 1 year ago

Pushed the update here:

https://github.com/kfuku52/amalgkit/commit/23a02c8c8c0f51fbd56a77d60b7577d6110d71e2

closing this for now, then.