czbiohub-sf / shrimPy

shrimPy: Smart High-throughput Robust Imaging & Measurement in Python
BSD 3-Clause "New" or "Revised" License
7 stars 1 forks source link

Unify CLI call signatures #65

Closed ieivanov closed 1 year ago

ieivanov commented 1 year ago

The CLI calls should have a standard signature, for example

mantis COMMAND INPUT_PATH OUTPUT_PATH CONFIG_PATH [OPTIONS]

or

mantis COMMAND -i INPUT_PATH -o OUTPUT_PATH --config CONFIG_PATH [OPTIONS]

We should make -h an alias for --help on all commands. In my opinion, the input/output/config paths should not be optional, i.e. have default values - you should always know where your data is coming from or going to.

I find the second example here more readable, but I also recognize that the first example is more of a standard practice. @edyoshikun @talonchandler @mattersoflight I welcome your input here.

Here are examples of our current CLI signatures:

(mantis) [ivan.ivanov@cpu-b-3 ~]$ mantis run-acquisition --help
Usage: mantis run-acquisition [OPTIONS]

  Acquire data using a settings file.

Options:
  -h, --help                Show this message and exit.
  --data-dirpath DIRECTORY  Directory where acquired data will be saved
                            [required]
  --name TEXT               Name of the acquisition  [required]
  --settings FILE           YAML file containing the acquisition settings
                            [required]
  --mm-app-path DIRECTORY   Path to Micro-manager installation directory which
                            will run the light-sheet acquisition  [default:
                            C:\Program Files\Micro-Manager-nightly]
  --mm-config-file FILE     Path to Micro-manager config file which will run
                            the light-sheet acquisition  [default:
                            C:\CompMicro_MMConfigs\mantis\mantis-LS.cfg]
(mantis) [ivan.ivanov@cpu-b-3 ~]$ mantis deskew --help
Usage: mantis deskew [OPTIONS] [INPUT_PATHS]... DESKEW_PARAM_PATH

  Deskews a single position across T and C axes using a parameter file
  generated by estimate_deskew.py

Options:
  -o, --output-path TEXT       Path to output.zarr
  -j, --num-processes INTEGER  Number of cores
  --help                       Show this message and exit.
(mantis) [ivan.ivanov@cpu-b-3 ~]$ mantis estimate-bleaching --help
Usage: mantis estimate-bleaching [OPTIONS] DATA_PATH

  Estimate bleaching from raw data

Options:
  -o, --output-folder TEXT  Path to output folder
  --help                    Show this message and exit.
talonchandler commented 1 year ago

Thanks for writing this @ieivanov.

I've changed my mind, and I now agree with your preference for signatures of the form:

mantis COMMAND -i INPUT_PATH -o OUTPUT_PATH --config CONFIG_PATH [OPTIONS]

This CLI style guide agrees: https://clig.dev/#arguments-and-flags "If you’ve got two or more arguments for different things, you’re probably doing something wrong."

talonchandler commented 1 year ago

I'm starting to chip away at this on WIP #75.

Here's my first pass at proposed calls. All comments are welcome.

# ACQUIRE
mantis run-acquisition -o ./ -n acq_name -c path/to/config.yaml

# CONVERT TO ZARR
iohub convert -i ./acq_name/acq_name_labelfree_1 -o ./acq_name_labelfree.zarr
iohub convert -i ./acq_name/acq_name_lightsheet_1 -o ./acq_name_lightsheet.zarr

# DESKEW
mantis estimate-deskew -i ./acq_name_lightsheet.zarr/0/0/0 -o ./deskew.yml
mantis deskew -i ./acq_name_lightsheet.zarr/*/*/* -c ./deskew_params.yml -o ./acq_name_lightsheet_deskewed.zarr

# UPCOMING CALLS AHEAD
# RECONSTRUCT
recorder reconstruct -i ./acq_name_labelfree.zarr/*/*/* -c ./recon.yml -o ./acq_name_labelfree_reconstructed.zarr

# REGISTER
mantis estimate-registration -ls ./acq_name_lightsheet_deskewed.zarr/0/0/0 -lf ./acq_name_labelfree_reconstructed.zarr/0/0/0 -o register.yml
mantis register -ls ./acq_name_lightsheet_deskewed.zarr -lf ./acq_name_lightsheet_deskewed.zarr -o ./acq_name_registerred.zarr

FYI @edyoshikun I found a way to let options take lists in click (like -i ./input.zarr/*/*/*)

ieivanov commented 1 year ago

Beautiful!

Minor omission - mantis register should also require an output argument, correct?

talonchandler commented 1 year ago

Good eye! I fixed it in my comment above.