Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start working on main ctapipe cli tool #2371

Closed
wants to merge 15 commits into from
Closed

Start working on main ctapipe cli tool #2371

wants to merge 15 commits into from

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jul 4, 2023

Fixes #850

I will also update the tools documentation in this PR, leaving as draft for now

setup.cfg Show resolved Hide resolved
@kosack
Copy link
Contributor

kosack commented Jul 4, 2023

I remember trying this in the past and there was something in our Tool base class that broke sub-commands. I guess whatever it was is fixed! Nice

@maxnoe
Copy link
Member Author

maxnoe commented Jul 4, 2023

At least I didn't find anything immediately wrong:

ctapipe process -i dataset://gamma_prod5.simtel.zst -o /tmp/foo.dl1.h5 --progress

for example works correctly here.

❯ ctapipe --help
Main entry point for ctapipe, provides other tools as subcommands

Subcommands
===========
Subcommands are launched as `ctapipe cmd [args]`. For information on using
subcommand 'cmd', do: `ctapipe cmd -h`.

process
    ctapipe event-wise data processing
apply-models
    Apply trained machine learning models
train-energy-regressor
    Train telescope-type-wise energy regression models
train-particle-classifier
    Train telescope-type-wise binary classficiation models
train-disp-regressor
    Train telescope-type-wise disp regression models for direction reconstruction
info
    Print information about ctapipe and the current installation
quickstart
    Create a directory with example configuration files

ctapipe/__main__.py Show resolved Hide resolved
ctapipe/tools/info.py Show resolved Hide resolved
"ctapipe.tools.train_disp_regressor.TrainDispRegressor",
"Train telescope-type-wise disp regression models for direction reconstruction",
),
"info": (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing "fileinfo" tool

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a few commits to convert ctapipe-fileinfo into a Tool and include it in this list. So this should be fixed now.

@kosack
Copy link
Contributor

kosack commented Jul 5, 2023

If you want to go full-hierarchy, you could also add a second layer for train:

% ctapipe train --help

Subcommands
===========
Subcommands are launched as `ctapipe train cmd [args]`. For information on using
subcommand 'cmd', do: `ctapipe train cmd -h`.

energy-regressor:
     Train energy regression model
particle-classifier:
    Train telescope-type-wise binary classficiation models
disp-regressor:
    Train telescope-type-wise disp regression models for direction reconstruction

% ctapipe train energy-regressor --help

Especially since we may add reco-uncertainty-regressor, event-type-classifier, and maybe others

- uses inspect.cleandoc() to properly strip away whitespace
- reformat a few to avoid lines longer than 80 cols
@kosack
Copy link
Contributor

kosack commented Aug 3, 2023

A few things still missing here:

  • we lose tab completion. Not a huge problem, but this could be solved by adding a bash/fish completion script. There is still of course tab completion of the underlying tools (e.g. ctapipe-process)

  • the docstring for each tool is now in three places. Is there a way we can simplify this?

    • the top of each file (used in ctapipe-info --tools)
    • the docstring of each Tool class (used in the notebook repr and python help)
    • the hard-coded string in __main__.py

running `ctapipe-info --all` gave no results, as the run functino was
overwritten

Also removed provenance output, as not needed for this tool
prevented ctapipe-info --tools from displaying a message
@kosack
Copy link
Contributor

kosack commented Aug 3, 2023

One more issue to solve: commands that use positional arguments don't seem to work in sub-commands. For example:

Works:

ctapipe-fileinfo f1.h5 f2.h5

Fails:

ctapipe fileinfo f1.h5 f2.h5
# the main program fails, as it tries to interpret the last two files as commands.

It still works using

ctapipe fileinfo --input-file f1.h5 --input-file f2.h5

but that defeats the purpose and makes it hard to do thing like ctapipe fileinfo *.h5.

This may also affect the merge tool, which also accepts positional args

@maxnoe maxnoe closed this Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make a top-level Tool called ctapipe
3 participants