jacg / petalo

PET scanner image reconstruction
1 stars 4 forks source link

makelor not working #25

Closed andLaing closed 1 year ago

andLaing commented 1 year ago

In the current HEAD version of the code the makelor binary does not launch properly.

Running cargo run --release --bin makelor -- --out ../mcred_data/sanity-comp/rust/reco-lors.h5 ../mc_raw/sanity-dz1m-LXe40mm-noQrz-6/sanity-dz1m-LXe40mm-noQrz-6-185.h5 first-vertex yields the help output which indicates that the command is valid.

The problem is present since 7267a10b1985b8fa88f4b11f28de2e058c23fbd5 though I can't find anything obviously wrong compared to the clap documentation I've read so far.

jacg commented 1 year ago

If you just need to use makelor, the quick workaround seems to be to pass --out ... after INFILES.

If you want to fix it, well It looks like this might be a bug in clap which only manifests itself in the presence of all of

  1. vector of unnamed arguments
  2. named argument
  3. subcommand
  4. cargo

Here's a minimal example to demonstrate it:

use clap::Parser;

#[derive(clap::Parser)]
#[clap(name = "erm", about = "Doesn't accept NAMED before ANONYMOUS as advertised by USAGE")]
pub struct Cli {

    // Changing this from Vec<u32> to u32 makes the problem go away
    pub anonymous: Vec<u32>,

    #[clap(short, long)]
    pub named: u32,

    // Removing this makes the problem go away
    #[clap(subcommand)]
    reco: Subcommand,
}

#[derive(clap::Subcommand)]
enum Subcommand {
    SubA,
    SubB,
}

fn main() {
    Cli::parse();
    println!("All's well");
}

even though the last one is consistent with the clap-generated USAGE suggests:

USAGE:
    erm --named <NAMED> [ANONYMOUS]... <SUBCOMMAND>
andLaing commented 1 year ago

Almost like an old school magical mystery seg fault from C++. OK, so not a bug in our code but a limitation of clap. There's a newer version (3.2.22) than we have (3.2.20), should we try changing versions before we complain?

I get the error for the suggested order both with and without cargo for makelor in the master, that is:

where works and fails just means a normal flow vs. the help screen appears.

jacg commented 1 year ago

There's a newer version (3.2.22) than we have (3.2.20), should we try changing versions

Seems to give me the same problems.

before we complain?

I've already submitted an issue. No 4238.

I get the error for the suggested order both with and without cargo for makelor in the master, that is:

  • target/release/makelor src/io/test.h5 --out mklor-test.h5 first-vertex works
  • target/release/makelor --out mklor-test.h5 src/io/test.h5 first-vertex fails
  • cargo run --release --bin makelor -- src/io/test.h5 --out mklor-test.h5 first-vertex works
  • cargo run --release --bin makelor -- --out mklor-test.h5 src/io/test.h5 first-vertex fails

where works and fails just means a normal flow vs. the help screen appears.

Hmm. Same here. What happens if you paste the example I showed above into src/bin/erm.rs and try the four usage examples I showed?

I know that zsh has some options which sometimes mess with command lines which work fine in bash, and I'm using a fancily-configured zsh.

andLaing commented 1 year ago

What happens if you paste the example I showed above into src/bin/erm.rs and try the four usage examples I showed?

In the same order: works, fails, works, fails.

I know that zsh has some options which sometimes mess with command lines which work fine in bash, and I'm using a fancily-configured zsh.

I'm using the standard .zshrc with the only additions being a few aliases, direnv added and Julia and cargo added to the path

jacg commented 1 year ago

The problem was caused by the algorithm (first-vertex, bary-vertex, etc.) being interpreted as one of the arbitrary number of input-file positional arguments. Adding a clap option fixes this.

The interface might be improved, but it's not obvious how. At least the suggested usage now works.