berenslab / MorphoPy

GNU General Public License v3.0
34 stars 11 forks source link

Some suggestions for the command line interface #107

Closed mstimberg closed 4 years ago

mstimberg commented 4 years ago

(Issue opened as part of openjournals/joss-reviews#2339)

Testing the command line utility, I noticed that it handles a missing -f in a surprising way. If I want to run it on a file in a directory full of .swc files, but I accidentally forget the -f option, it will run on all files in the directory instead of giving an error message:

$ MorphoPy.py -c stats ./C4.swc

['/mnt/data/anaconda2/envs/morphopy/bin/MorphoPy.py', '-c', 'stats', './C4.swc']
##### Morphometric statistics #####
... Process File:  EC3-80604.CNG.rotated.swc
   branch_points    width  ...  tree_asymmetry                   filename
0          216.0  1001.32  ...        0.814848  EC3-80604.CNG.rotated.swc

[1 rows x 29 columns]

... Process File:  EC3-80604.CNG.swc
There are 2-3 soma points. The location and the radius of the soma is estimated based on their mean.
[...]

Maybe one could even get rid of the -f and -d options completely, and have MorphoPy check whether the given name is a file or directory?

Additional minor suggestions for the command line utility:

Screenshot

Visdoom commented 4 years ago

Hey @mstimberg, @avdaranyi and I discussed the points you made above and this is what we came up with: The first issue you rose deals with the -f/-d flag in our command-line tool. The default option is set to -d so this is why you found the directory to be read in even though no flag is provided. However, we agree that having to choose between the two input options -f for file or -d for a directory is unintuitive and prone to errors for the user. We, therefore, decided to merge this option into a -i for input flag. This flag will be mandatory so it will create an error if it is left out and the script will automatically detect the nature of the input (folder vs file).

Secondly, the sys.argv is meant as a help for the user to see what parameters have been passed to the tool. As the command gets long it might lead to typos that are hard to spot. This is the reason why we'd prefer to keep it in the output but ultimately we do not have a strong opinion on this.

And lastly, we will slim down the error message to show the error and then a hint at how to open the --help interface. This should make the output easier to read.

mstimberg commented 4 years ago

All changes look good to me, I think the -i flag is a good solution.

Secondly, the sys.argv is meant as a help for the user to see what parameters have been passed to the tool. As the command gets long it might lead to typos that are hard to spot. This is the reason why we'd prefer to keep it in the output but ultimately we do not have a strong opinion on this.

I don't have a strong opinion on this either, happy with leaving it in if that's what you prefer.