Skip to content

Edge cases and error handling for the squeeze morph #227

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

Merged
merged 3 commits into from
Jul 2, 2025
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
23 changes: 23 additions & 0 deletions news/squeeze_lims.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* Error thrown when squeeze morph given improper inputs.

**Changed:**

* Squeeze morph now removes duplicate/repeated and trailing commas before parsing.

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
22 changes: 14 additions & 8 deletions src/diffpy/morph/morphapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ def custom_error(self, msg):
"a0+a1*x+a2*x^2+...a_n*x^n."
"n is dependent on the number of values in the "
"user-inputted comma-separated list. "
"Repeated and trailing commas are removed before parsing."
"When this option is enabled, --hshift is disabled. "
"When n>1, --stretch is disabled. "
"See online documentation for more information."
Expand Down Expand Up @@ -522,6 +523,7 @@ def single_morph(

# Squeeze
squeeze_poly_deg = -1
squeeze_dict_in = {}
if opts.squeeze is not None:
# Handles both list and csv input
if (
Expand All @@ -537,12 +539,19 @@ def single_morph(
):
opts.squeeze = opts.squeeze[1:-1]
squeeze_coeffs = opts.squeeze.strip().split(",")
squeeze_dict_in = {}
for idx, coeff in enumerate(squeeze_coeffs):
squeeze_dict_in.update({f"a{idx}": float(coeff)})
squeeze_poly_deg = len(squeeze_coeffs) - 1
idx = 0
for _, coeff in enumerate(squeeze_coeffs):
if coeff.strip() != "":
try:
squeeze_dict_in.update({f"a{idx}": float(coeff)})
idx += 1
except ValueError:
parser.error(f"{coeff} could not be converted to float.")
squeeze_poly_deg = len(squeeze_dict_in.keys())
chain.append(morphs.MorphSqueeze())
config["squeeze"] = squeeze_dict_in
# config["extrap_index_low"] = None
# config["extrap_index_high"] = None
refpars.append("squeeze")
# Scale
if opts.scale is not None:
Expand Down Expand Up @@ -679,10 +688,7 @@ def single_morph(
morph_inputs.update({"hshift": hshift_in, "vshift": vshift_in})
# More complex input morph parameters are only displayed conditionally
if opts.squeeze is not None:
squeeze_coeffs = opts.squeeze.strip().split(",")
squeeze_dict = {}
for idx, coeff in enumerate(squeeze_coeffs):
squeeze_dict.update({f"a{idx}": float(coeff)})
squeeze_dict = squeeze_dict_in.copy()
for idx, _ in enumerate(squeeze_dict):
morph_inputs.update({f"squeeze a{idx}": squeeze_dict[f"a{idx}"]})
if pymorphs is not None:
Expand Down
52 changes: 51 additions & 1 deletion tests/test_morphapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,27 +105,43 @@ def test_parser_numerical(self, setup_parser):
) # Check correct value parsed
assert len(n_args) == 1 # Check one leftover

def test_parser_systemexits(self, setup_parser):
def test_parser_systemexits(self, capsys, setup_parser):
# ###Basic tests for any variety of morphing###

# Ensure only two pargs given for morphing
(opts, pargs) = self.parser.parse_args(["toofewfiles"])
with pytest.raises(SystemExit):
single_morph(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "You must supply FILE1 and FILE2." in err
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "You must supply FILE and DIRECTORY." in err
(opts, pargs) = self.parser.parse_args(["too", "many", "files"])
with pytest.raises(SystemExit):
single_morph(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert (
"Too many arguments. Make sure you only supply FILE1 and FILE2."
in err
)
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert (
"Too many arguments. You must only supply a FILE and a DIRECTORY."
in err
)

# Make sure rmax greater than rmin
(opts, pargs) = self.parser.parse_args(
[f"{nickel_PDF}", f"{nickel_PDF}", "--rmin", "10", "--rmax", "1"]
)
with pytest.raises(SystemExit):
single_morph(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "rmin must be less than rmax" in err

# ###Tests exclusive to multiple morphs###
# Make sure we save to a directory that exists
Expand All @@ -140,8 +156,12 @@ def test_parser_systemexits(self, setup_parser):
)
with pytest.raises(SystemExit):
single_morph(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "Unable to save to designated location." in err
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "is not a directory." in err

# Ensure first parg is a FILE and second parg is a DIRECTORY
(opts, pargs) = self.parser.parse_args(
Expand All @@ -152,15 +172,21 @@ def test_parser_systemexits(self, setup_parser):
(opts, pargs) = self.parser.parse_args(
[f"{testsequence_dir}", f"{testsequence_dir}"]
)
_, err = capsys.readouterr()
assert "is not a directory." in err
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "is not a file." in err

# Try sorting by non-existing field
(opts, pargs) = self.parser.parse_args(
[f"{nickel_PDF}", f"{testsequence_dir}", "--sort-by", "fake_field"]
)
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "The requested field is missing from a PDF file header." in err
(opts, pargs) = self.parser.parse_args(
[
f"{nickel_PDF}",
Expand All @@ -173,6 +199,8 @@ def test_parser_systemexits(self, setup_parser):
)
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "The requested field was not found in the metadata file." in err

# Try plotting an unknown parameter
(opts, pargs) = self.parser.parse_args(
Expand All @@ -185,6 +213,8 @@ def test_parser_systemexits(self, setup_parser):
)
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "Cannot find specified plot parameter. No plot shown." in err

# Try plotting an unrefined parameter
(opts, pargs) = self.parser.parse_args(
Expand All @@ -197,6 +227,26 @@ def test_parser_systemexits(self, setup_parser):
)
with pytest.raises(SystemExit):
multiple_targets(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert (
"The plot parameter is missing values for at "
"least one morph and target pair. "
"No plot shown." in err
)

# Pass a non-float list to squeeze
(opts, pargs) = self.parser.parse_args(
[
f"{nickel_PDF}",
f"{nickel_PDF}",
"--squeeze",
"1,a,0",
]
)
with pytest.raises(SystemExit):
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we using sys.exit and not a proper exception? In general, this is to be avoided as it bypasses all the nice exception handling you get for free in Python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a test for the CLI, and the intended behavior is that once we receive a ValueError from the 'a' character, the parser raises an error, and the CLI program exits with value 1.

Copy link
Collaborator Author

@Sparks29032 Sparks29032 Jul 1, 2025

Choose a reason for hiding this comment

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

The line parser.error(f"{coeff} could not be converted to float.") (line 549 on 'morphapp.py') is calling the in-built option parser function to handle the error once we detect a ValueError on the Python side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, but we should ensure that the test is capturing this particular error, and not necessarily system exiting elsewhere. Let me put a new commit in a bit.

single_morph(self.parser, opts, pargs, stdout_flag=False)
_, err = capsys.readouterr()
assert "a could not be converted to float." in err

def test_morphsequence(self, setup_morphsequence):
# Parse arguments sorting by field
Expand Down
4 changes: 3 additions & 1 deletion tests/test_morphio.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,9 @@ def test_morphsqueeze_outputs(self, setup, tmp_path):
"--scale",
"2",
"--squeeze",
"0,-0.001,-0.0001,0.0001",
# Ignore duplicate commas and trailing commas
# Handle spaces and non-spaces
"0,, ,-0.001, -0.0001,0.0001,",
"--stretch",
"1",
"--hshift",
Expand Down
Loading