jotech / gapseq

Informed prediction and analysis of bacterial metabolic pathways and genome-scale networks
GNU General Public License v3.0
153 stars 32 forks source link

Issue with options when calling subcommand for "gapseq doall". #192

Closed ArnaudBelcour closed 1 year ago

ArnaudBelcour commented 1 year ago

Hi,

First, thank you for gapseq, it is really useful.

I have a small issue with options given to the sub command doall of gapseq on Ubuntu 22.04.

When calling ./gapseq -v, I have an expected result of:

gapseq version: 1.2 355da8ab

But when I try to call ./gapseq doall -v, I have:

readlink: missing operand
Try 'readlink --help' for more information.
   __ _  __ _ _ __  ___  ___  __ _ 
  / _` |/ _` | '_ \/ __|/ _ \/ _` |
 | (_| | (_| | |_) \__ \  __/ (_| |
  \__, |\__,_| .__/|___/\___|\__, |
  |___/      |_|                |_|

Informed prediction and analysis of bacterial metabolic pathways and genome-scale networks

Usage:
  gapseq test
  gapseq (find | find-transport | draft | fill | doall | adapt) (...)
  gapseq doall (genome) [medium] [Bacteria|Archaea]
  gapseq find (-p pathways | -e enzymes) [-b bitscore] (genome)
  gapseq find-transport [-b bitscore] (genome)
  gapseq draft (-r reactions | -t transporter -c genome -p pathways) [-b pos|neg|archaea|auto]
  gapseq medium (-m draft -p pathways) [-c manual_fluxes - o output_file]
  gapseq fill (-m draft -n medium -c rxn_weights -g rxn_genes)
  gapseq adapt (-a reactions/pathways | -r reactions/pathways| -w growh_compounds) -m model (-g rxn_genes, -c rxn_weights, -b reaction_blast_file)

Examples:
  gapseq test
  gapseq doall toy/ecoli.fna.gz
  gapseq doall toy/myb71.fna.gz dat/media/TSBmed.csv
  gapseq find -p chitin toy/myb71.fna.gz
  gapseq find -p all toy/myb71.fna.gz
  gapseq find-transport toy/myb71.fna.gz
  gapseq draft -r toy/ecoli-all-Reactions.tbl -t toy/ecoli-Transporter.tbl -c toy/ecoli.fna.gz -p toy/ecoli-all-Pathways.tbl
  gapseq medium -m toy/ecoli-draft.RDS -p toy/ecoli-all-Pathways.tbl
  gapseq fill -m toy/ecoli-draft.RDS -n dat/media/ALLmed.csv -c toy/ecoli-rxnWeights.RDS -g toy/ecoli-rxnXgenes.RDS
  gapseq adapt -a 14DICHLORBENZDEG-PWY -m toy/myb71.RDS
  gapseq adapt -m toy/myb71.RDS -w cpd00089:TRUE -c toy/myb71-rxnWeights.RDS -g toy/myb71-rxnXgenes.RDS -b toy/myb71-all-Reactions.tbl

Options:
  test            Testing dependencies and basic functionality of gapseq.
  find            Pathway analysis, try to find enzymes based on homology.
  find-transport  Search for transporters based on homology.
  draft           Draft model construction based on results from find and find-transport.
  medium          (gapfill-)Medium prediction based on results from find and draft
  fill            Gap filling of a model.
  doall           Combine find, find-transport, draft, (medium,) and fill.
  adapt           Add or remove reactions or pathways.
  -v              Show version.
  -h              Show this screen.
  -n              Enable noisy verbose mode.

I encountered this error while trying to add the -K option to gapseq doall(following the reading of this issue https://github.com/jotech/gapseq/issues/77). But it also happens, when I use the option -n to have a more verbose log with command like ./gapseq doall -n genome.fasta.

The error comes from the fact that readlink tries to read the parameter. I think I manage to solve the issue (by modifying the optind variables) and to add the -K argument to gapseq doall. Would you be interested that I create a Pull Request about it?

jotech commented 1 year ago

hi @ArnaudBelcour Thank you for your thoughtful comment! You are right; the current approach does not match the expected behavior.

And sure, it would be great to have a pull request for this!

The only thing why I am a bit hesitating is that gapseq doall is supposed to perform a default and recommended settings. Adding parameters for all sub-processes would probably add too much complexity, and for these use cases, advanced users should, in my opinion, better call the sub-processes like gapseq find or gapseq fill directly.

For -K, it makes total sense to add it, and maybe also for -v although I am not so completely sure here because doall starts the pipeline and is not primarily for getting general information and version numbers. What do you think?

ArnaudBelcour commented 1 year ago

I understand your point of using only default option with gapseq doall and yes, only the -K option could be added. I will make this minimal modification and we can discuss it on the PR.

For the other option, I was more thinking of -n, which allows to have a more verbose output when using gapseq doall. Because indeed, -v is not very useful to such command, but I was using it as the difference of outputs were the most striking.

jotech commented 1 year ago

I agree that -v and -K both make sense for gapseq doall!

If you still want to implement it, feel free to open a PR, otherwise, I can also do it.