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

Improve some CLI errors and docs #6088

Merged
merged 4 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/DevGuide/PythonBindings.md
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,12 @@ See \ref spectre_using_python "Using SpECTRE's Python"
subdirectory structures. Passing many files to a script is easy for the user
by using a glob (note: don't take the glob as a string argument, take the
expanded list of files directly using `click.argument(..., nargs=-1,
type=click.Path(...))`).
type=click.Path(...), required=True)`).
Note that Click recommends to avoid `required=True` here but to [gracefully
degrade to a noop](https://click.palletsprojects.com/en/8.1.x/arguments/#variadic-arguments)
instead, but that's confusing for the user.
- Never overwrite or delete files without prompting the user or asking them to
run with `--force`.
- When the input to a script is empty, [gracefully degrade to a
noop](https://click.palletsprojects.com/en/8.1.x/arguments/#variadic-arguments).
- When the user did not specify an option, print possible values for it and
return instead of raising an exception. For example, print the subfile names
in an H5 file if no subfile name was specified. This allows the user to make
Expand Down
17 changes: 9 additions & 8 deletions src/IO/H5/Python/CombineH5.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import spectre.IO.H5 as spectre_h5
from spectre.IO.H5.CombineH5Dat import combine_h5_dat_command
from spectre.support.CliExceptions import RequiredChoiceError


@click.group(name="combine-h5")
Expand All @@ -25,6 +26,7 @@ def combine_h5_command():
"h5files",
type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
nargs=-1,
required=True,
)
@click.option(
"--subfile-name",
Expand Down Expand Up @@ -66,17 +68,16 @@ def combine_h5_vol_command(h5files, subfile_name, output, check_src):
time steps (e.g. from multiple segments of a simulation). All input H5 files
must contain the same set of observation IDs.
"""
# CLI scripts should be noops when input is empty
if not h5files:
return

# Print available subfile names and exit
if not subfile_name:
spectre_file = spectre_h5.H5File(h5files[0], "r")
import rich.columns

rich.print(rich.columns.Columns(spectre_file.all_vol_files()))
return
raise RequiredChoiceError(
(
"Specify '--subfile-name' / '-d' to select a"
" subfile containing volume data."
),
choices=spectre_file.all_vol_files(),
)

if not output.endswith(".h5"):
output += ".h5"
Expand Down
5 changes: 1 addition & 4 deletions src/IO/H5/Python/CombineH5Dat.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@ def combine_h5_dat(h5files, output, force):
output: Output filename. An extension '.h5' will be added if not present.
force: If specified, overwrite output file if it already exists
"""

if len(h5files) == 0:
return

# Copy first input file to output file
if not output.endswith(".h5"):
output += ".h5"
Expand Down Expand Up @@ -72,6 +68,7 @@ def combine_h5_dat(h5files, output, force):
readable=True,
),
nargs=-1,
required=True,
)
@click.option(
"--output",
Expand Down
1 change: 1 addition & 0 deletions src/IO/H5/Python/DeleteSubfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"h5files",
type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
nargs=-1,
required=True,
)
@click.option(
"--subfile",
Expand Down
20 changes: 12 additions & 8 deletions src/Visualization/Python/GenerateXdmf.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import numpy as np
import rich

from spectre.support.CliExceptions import RequiredChoiceError
from spectre.Visualization.ReadH5 import available_subfiles

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -303,23 +304,25 @@ def generate_xdmf(
coordinates: Optional. Name of coordinates dataset. Default:
"InertialCoordinates".
"""
# CLI scripts should be noops when input is empty
if not h5files:
return

h5files = [(h5py.File(filename, "r"), filename) for filename in h5files]

if not subfile_name:
import rich.columns

subfiles = available_subfiles(
(h5file for h5file, _ in h5files), extension=".vol"
)
if len(subfiles) == 1:
subfile_name = subfiles[0]
logger.info(
f"Selected subfile {subfile_name} (the only available one)."
)
else:
rich.print(rich.columns.Columns(subfiles))
return
raise RequiredChoiceError(
(
"Specify '--subfile-name' / '-d' to select a"
" subfile containing volume data."
),
choices=subfiles,
)

if not subfile_name.endswith(".vol"):
subfile_name += ".vol"
Expand Down Expand Up @@ -445,6 +448,7 @@ def generate_xdmf(
"h5files",
type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
nargs=-1,
required=True,
)
@click.option(
"--output",
Expand Down
46 changes: 32 additions & 14 deletions src/Visualization/Python/OpenVolfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@
import rich

import spectre.IO.H5 as spectre_h5
from spectre.support.CliExceptions import RequiredChoiceError
from spectre.Visualization.ReadH5 import list_observations, select_observation

logger = logging.getLogger(__name__)


def open_volfiles(
h5_files: Iterable[str], subfile_name: str, obs_id: Optional[int] = None
Expand Down Expand Up @@ -87,6 +90,7 @@ def decorator(f):
@click.argument(
"h5_files",
nargs=-1,
required=True,
type=click.Path(
exists=True, file_okay=True, dir_okay=False, readable=True
),
Expand All @@ -95,7 +99,8 @@ def decorator(f):
"--subfile-name",
"-d",
help=(
"Name of subfile within h5 file containing volume data to plot."
"Name of subfile within H5 file containing volume data to plot."
" Optional if the H5 files have only one subfile."
),
)
@click.option(
Expand All @@ -114,7 +119,8 @@ def decorator(f):
"in the volume data file, such as 'Shift_x'. "
"Also accepts glob patterns like 'Shift_*'."
)
+ (" Can be specified multiple times." if multiple_vars else ""),
+ (" Can be specified multiple times." if multiple_vars else "")
+ (" [required]" if vars_required else ""),
)
@click.option(
"--list-observations",
Expand Down Expand Up @@ -154,21 +160,24 @@ def command(
time,
**kwargs,
):
# Script should be a noop if input files are empty
if not h5_files:
return

# Print available subfile names and exit
if not subfile_name:
import rich.columns

with spectre_h5.H5File(h5_files[0], "r") as open_h5_file:
available_subfiles = open_h5_file.all_vol_files()
if len(available_subfiles) == 1:
subfile_name = available_subfiles[0]
logger.info(
f"Selected subfile {subfile_name}"
" (the only available one)."
)
else:
rich.print(rich.columns.Columns(available_subfiles))
return
raise RequiredChoiceError(
(
"Specify '--subfile-name' / '-d' to select a"
" subfile containing volume data."
),
choices=available_subfiles,
)

# Print available observations/times and exit
if list_times:
Expand All @@ -189,6 +198,10 @@ def command(
)
if len(all_obs_ids) == 1:
obs_id, obs_time = all_obs_ids[0], all_obs_times[0]
logger.info(
f"Selected observation at t = {obs_time:g}"
" (the only available one)."
)
else:
raise click.UsageError(
"Specify '--step' or '--time' to select an"
Expand All @@ -210,21 +223,26 @@ def command(
obs_id or volfile.list_observation_ids()[0]
)
break
if list_vars or (vars_required and not vars_patterns):
if list_vars:
import rich.columns

rich.print(rich.columns.Columns(all_vars))
return
elif vars_required and not vars_patterns:
raise RequiredChoiceError(
"Specify '--var' / '-y' to select a variable to plot.",
choices=all_vars,
)
# Expand globs in vars
vars = []
if not multiple_vars:
vars_patterns = [vars_patterns] if vars_patterns else []
for var_pattern in vars_patterns:
matched_vars = fnmatch.filter(all_vars, var_pattern)
if not matched_vars:
raise click.UsageError(
f"The pattern '{var_pattern}' matches no variables. "
f"Available variables are: {all_vars}"
raise RequiredChoiceError(
f"The pattern '{var_pattern}' matches no variables.",
choices=all_vars,
)
vars.extend(matched_vars)
# Remove duplicates. Ordering is lost, but that's not important here.
Expand Down
7 changes: 5 additions & 2 deletions src/Visualization/Python/PlotAlongLine.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,16 @@ def points_on_line(

@click.command(name="along-line")
@open_volfiles_command(obs_id_required=False, multiple_vars=True)
# Line options
# These aren't marked "required" so the user can omit them when using options
# like '--list-vars'.
@click.option(
"--line-start",
"-A",
callback=parse_point,
help=(
"Coordinates of the start of the line through the volume data. "
"Specify as comma-separated list, e.g. '0,0,0'."
"Specify as comma-separated list, e.g. '0,0,0'. [required]"
),
)
@click.option(
Expand All @@ -63,7 +66,7 @@ def points_on_line(
callback=parse_point,
help=(
"Coordinates of the end of the line through the volume data. "
"Specify as comma-separated list, e.g. '1,0,0'."
"Specify as comma-separated list, e.g. '1,0,0'. [required]"
),
)
@click.option(
Expand Down
11 changes: 7 additions & 4 deletions src/Visualization/Python/PlotCce.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import matplotlib.pyplot as plt
import pandas as pd

from spectre.support.CliExceptions import RequiredChoiceError
from spectre.Visualization.Plot import (
apply_stylesheet_command,
show_or_save_plot_command,
Expand Down Expand Up @@ -205,10 +206,12 @@ def plot_cce_command(
)
cce_subfile = h5file.get(cce_subfile_name)
if cce_subfile is None:
raise click.UsageError(
f"Could not find Cce subfile {cce_subfile} in H5 file"
f" {h5_filename}. Available subfiles"
f" are:\n{available_subfiles(h5file, extension='cce')}"
raise RequiredChoiceError(
(
f"Could not find Cce subfile {cce_subfile} in H5 file"
f" {h5_filename}."
),
choices=cce_subfiles,
)

suffix = ".dat" if backward_cce_group is not None else ""
Expand Down
22 changes: 14 additions & 8 deletions src/Visualization/Python/PlotControlSystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import numpy as np
import pandas as pd

from spectre.support.CliExceptions import RequiredChoiceError
from spectre.Visualization.Plot import (
apply_stylesheet_command,
show_or_save_plot_command,
Expand All @@ -26,6 +27,7 @@
"reduction_files",
type=click.Path(exists=True, file_okay=True, dir_okay=False, readable=True),
nargs=-1,
required=True,
)
@click.option(
"--with-shape/--without-shape",
Expand Down Expand Up @@ -105,10 +107,12 @@ def check_control_system_dir(h5_filename: str):
h5file = h5py.File(h5_filename, "r")
control_system_dir = h5file.get("ControlSystems")
if control_system_dir is None:
raise click.UsageError(
"Unable to open group 'ControlSystems' from h5 file"
f" {h5_filename}. Available subfiles are:\n"
f" {available_subfiles(h5file, extension='.dat')}"
raise RequiredChoiceError(
(
"Unable to open group 'ControlSystems' from h5 file"
f" {h5_filename}."
),
choices=available_subfiles(h5file, extension=".dat"),
)

# No size control
Expand All @@ -128,10 +132,12 @@ def check_control_system_file(
)
subfile = h5file.get(subfile_path)
if subfile_path is None:
raise click.UsageError(
f"Unable to open control system file '{subfile_path}'"
f" from h5 file {h5_filename}. Available subfiles are:\n"
f" {available_subfiles(h5file, extension='.dat')}"
raise RequiredChoiceError(
(
f"Unable to open control system subfile '{subfile_path}'"
f" from h5 file {h5_filename}."
),
choices=available_subfiles(h5file, extension=".dat"),
)

return to_dataframe(subfile).set_index("Time")
Expand Down
Loading
Loading