From f23aca2e06f11ea39a177ba55695030562e26bb8 Mon Sep 17 00:00:00 2001 From: Toon Verstraelen Date: Thu, 11 Jul 2024 11:25:03 +0200 Subject: [PATCH 1/3] Add command-line option --allow-changes --- iodata/__main__.py | 19 ++++++-- iodata/test/test_cli.py | 99 ++++++++++++++++++++++++----------------- 2 files changed, 73 insertions(+), 45 deletions(-) diff --git a/iodata/__main__.py b/iodata/__main__.py index f5478a32..f79b628e 100755 --- a/iodata/__main__.py +++ b/iodata/__main__.py @@ -83,6 +83,14 @@ def parse_args(): parser.add_argument( "-o", "--outfmt", help="Select the output format, overrides automatic detection." ) + parser.add_argument( + "-c", + "--allow-changes", + default=False, + action="store_true", + help="Allow reorganizing the input data to make it comatible with the output format. " + "Warnings will be emitted for all changes made.", + ) parser.add_argument( "-m", "--many", @@ -95,7 +103,7 @@ def parse_args(): return parser.parse_args() -def convert(infn, outfn, many, infmt, outfmt): +def convert(infn: str, outfn: str, many: bool, infmt: str, outfmt: str, allow_changes: bool): """Convert file from one format to another. Parameters @@ -110,12 +118,15 @@ def convert(infn, outfn, many, infmt, outfmt): The input format. outfmt The output format. + allow_changes + Allow prepare_dump functions to modify the data + to make it compatible with the output format. """ if many: - dump_many(load_many(infn, fmt=infmt), outfn, fmt=outfmt) + dump_many(load_many(infn, fmt=infmt), outfn, allow_changes=allow_changes, fmt=outfmt) else: - dump_one(load_one(infn, fmt=infmt), outfn, fmt=outfmt) + dump_one(load_one(infn, fmt=infmt), outfn, allow_changes=allow_changes, fmt=outfmt) def main(): @@ -124,7 +135,7 @@ def main(): np.seterr(divide="raise", over="raise", invalid="raise") args = parse_args() - convert(args.input, args.output, args.many, args.infmt, args.outfmt) + convert(args.input, args.output, args.many, args.infmt, args.outfmt, args.allow_changes) if __name__ == "__main__": diff --git a/iodata/test/test_cli.py b/iodata/test/test_cli.py index 848429f1..dcc7a9e8 100644 --- a/iodata/test/test_cli.py +++ b/iodata/test/test_cli.py @@ -18,59 +18,91 @@ # -- """Unit tests for iodata.__main__.""" -import functools import os import subprocess import sys +from functools import partial from importlib.resources import as_file, files +from warnings import warn +import pytest from numpy.testing import assert_allclose, assert_equal -from ..__main__ import convert +from ..__main__ import convert as convfn from ..api import load_many, load_one +from ..utils import PrepareDumpError, PrepareDumpWarning + + +def _convscript(infn: str, outfn: str, many: bool, infmt: str, outfmt: str, allow_changes: bool): + args = [sys.executable, "-m", "iodata.__main__", infn, outfn] + if many: + args.append("-m") + if infmt is not None: + args.append(f"--infmt={infmt}") + if outfmt is not None: + args.append(f"--outfmt={outfmt}") + if allow_changes: + args.append("-c") + try: + subprocess.run(args, check=True) + except subprocess.CalledProcessError as exc: + raise PrepareDumpError("Translated error") from exc + if allow_changes: + warn(PrepareDumpWarning("Some message"), stacklevel=2) def _check_convert_one(myconvert, tmpdir): outfn = os.path.join(tmpdir, "tmp.xyz") with as_file(files("iodata.test.data").joinpath("hf_sto3g.fchk")) as infn: - myconvert(infn, outfn) + myconvert(infn, outfn, allow_changes=False) iodata = load_one(outfn) assert iodata.natom == 2 assert_equal(iodata.atnums, [9, 1]) assert_allclose(iodata.atcoords, [[0.0, 0.0, 0.190484394], [0.0, 0.0, -1.71435955]]) -def test_convert_one_autofmt(tmpdir): - myconvert = functools.partial(convert, many=False, infmt=None, outfmt=None) - _check_convert_one(myconvert, tmpdir) +def _check_convert_one_changes(myconvert, tmpdir): + outfn = os.path.join(tmpdir, "tmp.mkl") + with as_file(files("iodata.test.data").joinpath("hf_sto3g.fchk")) as infn: + with pytest.raises(PrepareDumpError): + myconvert(infn, outfn, allow_changes=False) + assert not os.path.isfile(outfn) + with pytest.warns(PrepareDumpWarning): + myconvert(infn, outfn, allow_changes=True) + iodata = load_one(outfn) + assert iodata.natom == 2 + assert_equal(iodata.atnums, [9, 1]) + assert_allclose(iodata.atcoords, [[0.0, 0.0, 0.190484394], [0.0, 0.0, -1.71435955]]) -def test_convert_one_manfmt(tmpdir): - myconvert = functools.partial(convert, many=False, infmt="fchk", outfmt="xyz") +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_one_autofmt(tmpdir, convert): + myconvert = partial(convfn, many=False, infmt=None, outfmt=None) _check_convert_one(myconvert, tmpdir) -def test_script_one_autofmt(tmpdir): - def myconvert(infn, outfn): - subprocess.run([sys.executable, "-m", "iodata.__main__", infn, outfn], check=True) +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_one_autofmt_changes(tmpdir, convert): + myconvert = partial(convert, many=False, infmt=None, outfmt=None) + _check_convert_one_changes(myconvert, tmpdir) - _check_convert_one(myconvert, tmpdir) +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_one_manfmt(tmpdir, convert): + myconvert = partial(convert, many=False, infmt="fchk", outfmt="xyz") + _check_convert_one(myconvert, tmpdir) -def test_script_one_manfmt(tmpdir): - def myconvert(infn, outfn): - subprocess.run( - [sys.executable, "-m", "iodata.__main__", infn, outfn, "-i", "fchk", "-o", "xyz"], - check=True, - ) - _check_convert_one(myconvert, tmpdir) +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_one_manfmt_changes(tmpdir, convert): + myconvert = partial(convert, many=False, infmt="fchk", outfmt="molekel") + _check_convert_one_changes(myconvert, tmpdir) def _check_convert_many(myconvert, tmpdir): outfn = os.path.join(tmpdir, "tmp.xyz") with as_file(files("iodata.test.data").joinpath("peroxide_relaxed_scan.fchk")) as infn: - myconvert(infn, outfn) + myconvert(infn, outfn, allow_changes=False) trj = list(load_many(outfn)) assert len(trj) == 13 for iodata in trj: @@ -80,28 +112,13 @@ def _check_convert_many(myconvert, tmpdir): assert_allclose(trj[5].atcoords[0], [0.0, 1.32466211, 0.0], atol=1e-5) -def test_convert_many_autofmt(tmpdir): - myconvert = functools.partial(convert, many=True, infmt=None, outfmt=None) +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_many_autofmt(tmpdir, convert): + myconvert = partial(convert, many=True, infmt=None, outfmt=None) _check_convert_many(myconvert, tmpdir) -def test_convert_many_manfmt(tmpdir): - myconvert = functools.partial(convert, many=True, infmt="fchk", outfmt="xyz") - _check_convert_many(myconvert, tmpdir) - - -def test_script_many_autofmt(tmpdir): - def myconvert(infn, outfn): - subprocess.run([sys.executable, "-m", "iodata.__main__", infn, outfn, "-m"], check=True) - - _check_convert_many(myconvert, tmpdir) - - -def test_script_many_manfmt(tmpdir): - def myconvert(infn, outfn): - subprocess.run( - [sys.executable, "-m", "iodata.__main__", infn, outfn, "-m", "-i", "fchk", "-o", "xyz"], - check=True, - ) - +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_many_manfmt(tmpdir, convert): + myconvert = partial(convert, many=True, infmt="fchk", outfmt="xyz") _check_convert_many(myconvert, tmpdir) From aa2cc207afd587fed66e5773d4b02d1cf5019440 Mon Sep 17 00:00:00 2001 From: Toon Verstraelen Date: Sun, 14 Jul 2024 19:47:56 +0200 Subject: [PATCH 2/3] AI suggestions: spelling and extra unit tests --- iodata/__main__.py | 4 ++-- iodata/test/test_cli.py | 33 ++++++++++++++++++++++++++------- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/iodata/__main__.py b/iodata/__main__.py index f79b628e..4fa75fdc 100755 --- a/iodata/__main__.py +++ b/iodata/__main__.py @@ -88,8 +88,8 @@ def parse_args(): "--allow-changes", default=False, action="store_true", - help="Allow reorganizing the input data to make it comatible with the output format. " - "Warnings will be emitted for all changes made.", + help="Allow (not trivially reversible) conversion the input data to make it compatible " + "with the output format. Warnings will be emitted for all changes made.", ) parser.add_argument( "-m", diff --git a/iodata/test/test_cli.py b/iodata/test/test_cli.py index dcc7a9e8..37d71b7b 100644 --- a/iodata/test/test_cli.py +++ b/iodata/test/test_cli.py @@ -30,10 +30,11 @@ from ..__main__ import convert as convfn from ..api import load_many, load_one -from ..utils import PrepareDumpError, PrepareDumpWarning +from ..utils import FileFormatError, PrepareDumpError, PrepareDumpWarning def _convscript(infn: str, outfn: str, many: bool, infmt: str, outfmt: str, allow_changes: bool): + """Simulate the convert function by calling iodata-convert in a subprocess.""" args = [sys.executable, "-m", "iodata.__main__", infn, outfn] if many: args.append("-m") @@ -43,12 +44,16 @@ def _convscript(infn: str, outfn: str, many: bool, infmt: str, outfmt: str, allo args.append(f"--outfmt={outfmt}") if allow_changes: args.append("-c") - try: - subprocess.run(args, check=True) - except subprocess.CalledProcessError as exc: - raise PrepareDumpError("Translated error") from exc - if allow_changes: - warn(PrepareDumpWarning("Some message"), stacklevel=2) + cp = subprocess.run(args, capture_output=True, check=False, encoding="utf8") + if cp.returncode == 0: + if allow_changes and "PrepareDumpWarning" in cp.stderr: + warn(PrepareDumpWarning(cp.stderr), stacklevel=2) + else: + if "PrepareDumpError" in cp.stderr: + raise PrepareDumpError(cp.stderr) + if "FileFormatError" in cp.stderr: + raise FileFormatError(cp.stderr) + raise RuntimeError(f"Failure not processed.\n{cp.stderr}") def _check_convert_one(myconvert, tmpdir): @@ -93,6 +98,20 @@ def test_convert_one_manfmt(tmpdir, convert): _check_convert_one(myconvert, tmpdir) +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_one_nonexisting_infmt(tmpdir, convert): + myconvert = partial(convert, many=False, infmt="blablabla", outfmt="xyz") + with pytest.raises(FileFormatError): + _check_convert_one(myconvert, tmpdir) + + +@pytest.mark.parametrize("convert", [convfn, _convscript]) +def test_convert_one_nonexisting_outfmt(tmpdir, convert): + myconvert = partial(convert, many=False, infmt="fchk", outfmt="blablabla") + with pytest.raises(FileFormatError): + _check_convert_one(myconvert, tmpdir) + + @pytest.mark.parametrize("convert", [convfn, _convscript]) def test_convert_one_manfmt_changes(tmpdir, convert): myconvert = partial(convert, many=False, infmt="fchk", outfmt="molekel") From 0a4d9790444ad0456ac885e544ace893e686f671 Mon Sep 17 00:00:00 2001 From: Toon Verstraelen Date: Sun, 14 Jul 2024 20:20:34 +0200 Subject: [PATCH 3/3] More AI Fixes --- iodata/__main__.py | 14 +++++++++++--- iodata/test/test_cli.py | 10 +++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/iodata/__main__.py b/iodata/__main__.py index 4fa75fdc..47f5f49c 100755 --- a/iodata/__main__.py +++ b/iodata/__main__.py @@ -20,6 +20,7 @@ """CLI for file conversion.""" import argparse +from typing import Optional import numpy as np @@ -31,7 +32,7 @@ __version__ = "0.0.0.post0" -__all__ = () +__all__ = ("convert",) DESCRIPTION = """\ @@ -88,7 +89,7 @@ def parse_args(): "--allow-changes", default=False, action="store_true", - help="Allow (not trivially reversible) conversion the input data to make it compatible " + help="Allow (not trivially reversible) conversion of the input data to make it compatible " "with the output format. Warnings will be emitted for all changes made.", ) parser.add_argument( @@ -103,7 +104,14 @@ def parse_args(): return parser.parse_args() -def convert(infn: str, outfn: str, many: bool, infmt: str, outfmt: str, allow_changes: bool): +def convert( + infn: str, + outfn: str, + many: bool = False, + infmt: Optional[str] = None, + outfmt: Optional[str] = None, + allow_changes: bool = False, +): """Convert file from one format to another. Parameters diff --git a/iodata/test/test_cli.py b/iodata/test/test_cli.py index 37d71b7b..a0b1f131 100644 --- a/iodata/test/test_cli.py +++ b/iodata/test/test_cli.py @@ -23,6 +23,7 @@ import sys from functools import partial from importlib.resources import as_file, files +from typing import Optional from warnings import warn import pytest @@ -33,7 +34,14 @@ from ..utils import FileFormatError, PrepareDumpError, PrepareDumpWarning -def _convscript(infn: str, outfn: str, many: bool, infmt: str, outfmt: str, allow_changes: bool): +def _convscript( + infn: str, + outfn: str, + many: bool = False, + infmt: Optional[str] = None, + outfmt: Optional[str] = None, + allow_changes: bool = False, +): """Simulate the convert function by calling iodata-convert in a subprocess.""" args = [sys.executable, "-m", "iodata.__main__", infn, outfn] if many: