Skip to content

Commit

Permalink
Tidy code (stfc#81)
Browse files Browse the repository at this point in the history
* Simplify run_single_point function name

* Tidy CLI help, docstrings and tests

---------

Co-authored-by: ElliottKasoar <ElliottKasoar@users.noreply.github.com>
  • Loading branch information
ElliottKasoar and ElliottKasoar authored Mar 19, 2024
1 parent 3038a2d commit f4ef2e5
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 38 deletions.
20 changes: 14 additions & 6 deletions janus_core/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ def parse_dict_class(value: str):
TyperDict,
typer.Option(
parser=parse_dict_class,
help="Keyword arguments to pass to selected calculator [default: {}]",
help=(
"Keyword arguments to pass to selected calculator. For the default "
"architecture ('mace_mp'), {'model':'small'} is set by default "
"[default: {}]"
),
metavar="DICT",
),
]
Expand All @@ -92,7 +96,7 @@ def parse_dict_class(value: str):
parser=parse_dict_class,
help=(
"Keyword arguments to pass to ase.io.write when saving "
"results [default: {}]"
"results [default: {}]"
),
metavar="DICT",
),
Expand Down Expand Up @@ -162,9 +166,7 @@ def singlepoint(
calc_kwargs=calc_kwargs,
log_kwargs={"filename": log_file, "filemode": "w"},
)
s_point.run_single_point(
properties=properties, write_results=True, write_kwargs=write_kwargs
)
s_point.run(properties=properties, write_results=True, write_kwargs=write_kwargs)


@app.command()
Expand All @@ -184,7 +186,13 @@ def geomopt( # pylint: disable=too-many-arguments,too-many-locals
] = False,
vectors_only: Annotated[
bool,
typer.Option("--vectors-only", help="Allow only cell vectors to change"),
typer.Option(
"--vectors-only",
help=(
"Disable optimizing cell angles, so only cell vectors and atomic "
"positions are optimized. Requires --fully-opt to be set"
),
),
] = False,
read_kwargs: ReadKwargs = None,
calc_kwargs: CalcKwargs = None,
Expand Down
4 changes: 2 additions & 2 deletions janus_core/single_point.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class SinglePoint:
Read structure and structure name.
set_calculator(**kwargs)
Configure calculator and attach to structure.
run_single_point(properties=None)
run(properties=None)
Run single point calculations.
"""

Expand Down Expand Up @@ -298,7 +298,7 @@ def _clean_results(
else:
self._remove_invalid_props(self.struct, results, properties)

def run_single_point(
def run(
self,
properties: MaybeSequence[str] = (),
write_results: bool = False,
Expand Down
4 changes: 2 additions & 2 deletions tests/test_geom_opt.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def test_optimize(architecture, struct_path, expected, kwargs):
calc_kwargs={"model": MODEL_PATH},
)

init_energy = single_point.run_single_point("energy")["energy"]
init_energy = single_point.run("energy")["energy"]

atoms = optimize(single_point.struct, **kwargs)

Expand All @@ -64,7 +64,7 @@ def test_saving_struct(tmp_path):
calc_kwargs={"model": MODEL_PATH},
)

init_energy = single_point.run_single_point("energy")["energy"]
init_energy = single_point.run("energy")["energy"]

optimize(
single_point.struct,
Expand Down
12 changes: 0 additions & 12 deletions tests/test_mlip_calculators.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,3 @@ def test_model_model_paths():
model=MODEL_PATH,
model_paths=MODEL_PATH,
)
with pytest.raises(ValueError):
choose_calculator(
architecture="mace_mp",
model=MODEL_PATH,
model_paths=MODEL_PATH,
)
with pytest.raises(ValueError):
choose_calculator(
architecture="mace_off",
model=MODEL_PATH,
model_paths=MODEL_PATH,
)
26 changes: 11 additions & 15 deletions tests/test_single_point.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_potential_energy(
single_point = SinglePoint(
struct_path=struct_path, architecture="mace", calc_kwargs=calc_kwargs
)
results = single_point.run_single_point(properties)[prop_key]
results = single_point.run(properties)[prop_key]

# Check correct values returned
if idx is not None:
Expand All @@ -60,7 +60,7 @@ def test_single_point_none():
calc_kwargs={"model": MODEL_PATH},
)

results = single_point.run_single_point()
results = single_point.run()
for prop in ["energy", "forces", "stress"]:
assert prop in results

Expand All @@ -73,7 +73,7 @@ def test_single_point_clean():
calc_kwargs={"model": MODEL_PATH},
)

results = single_point.run_single_point()
results = single_point.run()
for prop in ["energy", "forces"]:
assert prop in results
assert "stress" not in results
Expand All @@ -89,7 +89,7 @@ def test_single_point_traj():
)

assert len(single_point.struct) == 2
results = single_point.run_single_point("energy")
results = single_point.run("energy")
assert results["energy"][0] == pytest.approx(-76.0605725422795)
assert results["energy"][1] == pytest.approx(-74.80419118083256)

Expand All @@ -107,7 +107,7 @@ def test_single_point_write():
)
assert "forces" not in single_point.struct.arrays

single_point.run_single_point(write_results=True)
single_point.run(write_results=True)

atoms = read_atoms(results_path)
assert atoms.get_potential_energy() is not None
Expand All @@ -126,9 +126,7 @@ def test_single_point_write_kwargs(tmp_path):
)
assert "forces" not in single_point.struct.arrays

single_point.run_single_point(
write_results=True, write_kwargs={"filename": results_path}
)
single_point.run(write_results=True, write_kwargs={"filename": results_path})
atoms = read(results_path)
assert atoms.get_potential_energy() is not None
assert "forces" in atoms.arrays
Expand All @@ -144,13 +142,11 @@ def test_single_point_write_nan(tmp_path):
calc_kwargs={"model": MODEL_PATH},
)

assert isfinite(single_point.run_single_point("energy")["energy"]).all()
assert isfinite(single_point.run("energy")["energy"]).all()
with pytest.raises(ValueError):
single_point.run_single_point("stress")
single_point.run("stress")

single_point.run_single_point(
write_results=True, write_kwargs={"filename": results_path}
)
single_point.run(write_results=True, write_kwargs={"filename": results_path})
atoms = read(results_path)
assert atoms.get_potential_energy() is not None
assert "forces" in atoms.calc.results
Expand All @@ -165,7 +161,7 @@ def test_invalid_prop():
calc_kwargs={"model": MODEL_PATH},
)
with pytest.raises(NotImplementedError):
single_point.run_single_point("invalid")
single_point.run("invalid")


def test_atoms():
Expand All @@ -178,7 +174,7 @@ def test_atoms():
calc_kwargs={"model": MODEL_PATH},
)
assert single_point.struct_name == "NaCl"
assert single_point.run_single_point("energy")["energy"] < 0
assert single_point.run("energy")["energy"] < 0


def test_default_atoms_name():
Expand Down
2 changes: 1 addition & 1 deletion tests/utils.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Utility functions for tests"""
"""Utility functions for tests."""

from pathlib import Path
from typing import Union
Expand Down

0 comments on commit f4ef2e5

Please sign in to comment.