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

Enable Ruff rule family "N" and "S" #3892

Merged
merged 73 commits into from
Aug 7, 2024
Merged
Changes from 1 commit
Commits
Show all changes
73 commits
Select commit Hold shift + click to select a range
c5f904c
enable ruff rule family N
DanielYang59 Jun 23, 2024
9e6aefb
fix NPY002 in code (tests to be fixed)
DanielYang59 Jun 23, 2024
6d7718a
fix NPY002 in tests
DanielYang59 Jun 23, 2024
e6b4414
replace random with np where np is imported
DanielYang59 Jun 23, 2024
5075261
fix miller index type
DanielYang59 Jun 23, 2024
560ef73
[Need Confirm] remove rnd seed and reduce distortion
DanielYang59 Jun 23, 2024
ccd484f
fix random seed
DanielYang59 Jun 23, 2024
e3cea79
correct replacement of randn with standard_normal
DanielYang59 Jun 23, 2024
39f6267
enable rule family S and format tweaks
DanielYang59 Jun 24, 2024
7f0ef83
suppress S501 error
DanielYang59 Jun 24, 2024
156bf36
suppress S105 errors
DanielYang59 Jun 24, 2024
62ede68
suppress S602
DanielYang59 Jun 24, 2024
af8af85
replace weak `sha1` hash with `sha256`
DanielYang59 Jun 24, 2024
0e19912
ignore S311 as pymatgen is not for cryptography
DanielYang59 Jun 24, 2024
e2e14d2
suppress S605 for trusted source
DanielYang59 Jun 24, 2024
26ccb85
NEED CONFIRM: fix S607 for the starting of a process with a partial e…
DanielYang59 Jun 24, 2024
33401d4
suppress S310 for trusted source
DanielYang59 Jun 24, 2024
ebb299f
suppress S608 for trusted source
DanielYang59 Jun 24, 2024
c52f162
NEED CONFIRMATION: rewrite subprocess without shell
DanielYang59 Jun 24, 2024
9d3c6da
suppress S301, but still enable it because pickle is known to have po…
DanielYang59 Jun 24, 2024
4947ff5
replace xml with defusedxml to fix S314
DanielYang59 Jun 24, 2024
8a75287
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jun 25, 2024
a51d075
Revert "replace xml with defusedxml to fix S314"
DanielYang59 Jun 25, 2024
84e9cdd
ignore S314
DanielYang59 Jun 25, 2024
02a7979
suppress S607 in tasks.py
DanielYang59 Jun 25, 2024
abf9edd
Revert "NEED CONFIRM: fix S607 for the starting of a process with a p…
DanielYang59 Jun 26, 2024
7d09bdf
Fix DTZ003 deprecated datetime API
DanielYang59 Jun 26, 2024
8c8b3e7
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jun 26, 2024
3cb9c71
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jun 26, 2024
d366898
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 1, 2024
ec5ea31
pre-commit auto-fixes
pre-commit-ci[bot] Jul 1, 2024
71b7af4
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 3, 2024
e1b5db3
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 12, 2024
732841d
update monty to fix datetime serialization
DanielYang59 Jul 13, 2024
67d1aa6
replace np.exceptions.RankWarning
DanielYang59 Jul 13, 2024
89e0d22
update monty in pyproject
DanielYang59 Jul 13, 2024
a4f2080
revert to np.RankWarning for now
DanielYang59 Jul 13, 2024
98f4501
revert accidental np.trapz change during merge
DanielYang59 Jul 13, 2024
f6a7275
pre-commit auto-fixes
pre-commit-ci[bot] Jul 13, 2024
2adac55
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 15, 2024
21022c6
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 17, 2024
9f3a8bc
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 18, 2024
d75e0c5
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 24, 2024
aa3d6cb
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Jul 28, 2024
134a11c
pre-commit auto-fixes
pre-commit-ci[bot] Jul 28, 2024
f82d0a3
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Aug 3, 2024
d0ef23d
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Aug 3, 2024
52e8735
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Aug 5, 2024
64a702a
suppress S202 for trusted source
DanielYang59 Aug 5, 2024
91b1ea6
ignore NPY201 for now
DanielYang59 Aug 5, 2024
5a923af
Revert "pre-commit auto-fixes"
DanielYang59 Aug 5, 2024
bc3caa9
pre-commit auto-fixes
pre-commit-ci[bot] Aug 5, 2024
57848ed
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Aug 5, 2024
94ca1fd
fix indentation
DanielYang59 Aug 5, 2024
638a75f
Revert "pre-commit auto-fixes"
DanielYang59 Aug 5, 2024
5723727
regenerate requirement.txt
DanielYang59 Aug 5, 2024
2cbff0b
fix indentation
DanielYang59 Aug 5, 2024
d65f4f4
rename single-letter p = subprocess.run
janosh Aug 5, 2024
de81808
replace stdlib random with numpy
DanielYang59 Aug 5, 2024
374f711
avoid assign single use rng
DanielYang59 Aug 5, 2024
2378f85
fix unit test
DanielYang59 Aug 5, 2024
0ffeac4
keep non-imperative
DanielYang59 Aug 5, 2024
fdf52f1
replace os.system with subprocess
DanielYang59 Aug 6, 2024
08ec70d
docstring and type tweaks for io.packmol
DanielYang59 Aug 6, 2024
69802bf
io.packmol format tweaks
DanielYang59 Aug 6, 2024
684520a
fix subprocess run usage for stdin file
DanielYang59 Aug 6, 2024
9370c6f
add return type in docstring
DanielYang59 Aug 6, 2024
d73aa3b
(feel free to revert) use list join over str concat
DanielYang59 Aug 6, 2024
2134e59
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Aug 6, 2024
bcf1c0b
Merge branch 'master' into enable-ruff-s-npy
DanielYang59 Aug 6, 2024
f84c906
use f-str
janosh Aug 7, 2024
701faac
map(str, box)
janosh Aug 7, 2024
33c1dcd
revert f-str
janosh Aug 7, 2024
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
105 changes: 52 additions & 53 deletions src/pymatgen/io/packmol.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
"""
This module provides a pymatgen I/O interface to packmol.
This module provides a pymatgen I/O interface to PACKMOL.

This adopts the minimal core I/O interface (see pymatgen/io/core).
In this case, only a two classes are used. PackmolSet(InputSet) is the container
class that provides a run() method for running packmol locally.
- PackmolSet provides a "run" method to run PACKMOL locally.

PackmolBoxGen(InputGenerator) provides a recipe for packing molecules into a
box, which returns a PackmolSet object.
- PackmolBoxGen provides "get_input_set" for packing molecules into a box,
which returns a PackmolSet object.

For the run() method to work, you need to install the packmol package
See http://m3g.iqm.unicamp.br/packmol or
http://leandro.iqm.unicamp.br/m3g/packmol/home.shtml
DanielYang59 marked this conversation as resolved.
Show resolved Hide resolved
for download and setup instructions. Note that packmol versions prior to 20.3.0
do not support paths with spaces.
After installation, you may need to manually add the path of the packmol
For the run() method to work, you need to install the PACKMOL package.
See http://m3g.iqm.unicamp.br/packmol for download and setup instructions.
After installation, you may need to add the path of the PACKMOL
executable to the PATH environment variable.

Note that PACKMOL versions prior to 20.3.0 do not support paths with spaces.
"""

from __future__ import annotations
Expand All @@ -41,18 +38,18 @@ class that provides a run() method for running packmol locally.


class PackmolSet(InputSet):
"""InputSet for the Packmol software. This class defines several attributes related to."""
"""InputSet for the PACKMOL software. This class defines several attributes related to."""

def run(self, path: PathLike, timeout=30):
"""Run packmol and write out the packed structure.
def run(self, path: PathLike, timeout: float = 30) -> None:
"""Run PACKMOL and write out the packed structure.

Args:
path: The path in which packmol input files are located.
timeout: Timeout in seconds.
path (PathLike): The path in which packmol input files are located.
timeout (float): Timeout in seconds.

Raises:
ValueError if packmol does not succeed in packing the box.
TimeoutExpiredError if packmold does not finish within the timeout.
ValueError: if packmol does not succeed in packing the box.
TimeoutExpiredError: if packmol does not finish within the timeout.
"""
wd = os.getcwd()
if not which("packmol"):
Expand All @@ -70,9 +67,9 @@ def run(self, path: PathLike, timeout=30):
timeout=timeout,
capture_output=True,
)
# this workaround is needed because packmol can fail to find
# a solution but still return a zero exit code
# see https://github.com/m3g/packmol/issues/28
# This workaround is needed because packmol can fail to find
# a solution but still return a zero exit code.
# See https://github.com/m3g/packmol/issues/28
if "ERROR" in proc.stdout.decode():
if "Could not open file." in proc.stdout.decode():
raise ValueError(
Expand All @@ -81,10 +78,11 @@ def run(self, path: PathLike, timeout=30):
)
msg = proc.stdout.decode().split("ERROR")[-1]
raise ValueError(f"Packmol failed with return code 0 and stdout: {msg}")

except subprocess.CalledProcessError as exc:
raise ValueError(f"Packmol failed with error code {exc.returncode} and stderr: {exc.stderr}") from exc
else:
with open(Path(path, self.stdoutfile), mode="w") as out:
with open(Path(path, self.stdoutfile), mode="w", encoding="utf-8") as out:
out.write(proc.stdout.decode())
finally:
os.chdir(wd)
Expand Down Expand Up @@ -120,12 +118,12 @@ def __init__(
like filenames, random seed, tolerance, etc.

Args:
tolerance: Tolerance for packmol, in Å.
seed: Random seed for packmol. Use a value of 1 (default) for deterministic
tolerance (float): Tolerance for packmol, in Å.
seed (int): Random seed for packmol. Use 1 (default) for deterministic
output, or -1 to generate a new random seed from the current time.
inputfile: Path to the input file. Default to 'packmol.inp'.
outputfile: Path to the output file. Default to 'output.xyz'.
stdoutfile: Path to the file where stdout will be recorded. Default to 'packmol.stdout'
inputfile (PathLike): Path to the input file. Default to "packmol.inp".
outputfile (PathLike): Path to the output file. Default to "packmol_out.xyz".
stdoutfile (PathLike): Path to the file where stdout will be recorded. Default to "packmol.stdout".
"""
self.inputfile = inputfile
self.outputfile = outputfile
Expand All @@ -142,18 +140,19 @@ def get_input_set(
"""Generate a Packmol InputSet for a set of molecules.

Args:
molecules: A list of dict containing information about molecules to pack
molecules (list[dict]): Information about molecules to pack
into the box. Each dict requires three keys:
1. "name" - the structure name
2. "number" - the number of that molecule to pack into the box
3. "coords" - Coordinates in the form of either a Molecule object or
a path to a file.

Example:
{"name": "water",
"number": 500,
"coords": "/path/to/input/file.xyz"}
box: A list of box dimensions xlo, ylo, zlo, xhi, yhi, zhi, in Å. If set to None
1. "name" - the structure name.
2. "number" - the number of that molecule to pack into the box.
3. "coords" - Coordinates in the form of either a Molecule
object or a path to a file.
Example:
{
"name": "water",
"number": 500,
"coords": "/path/to/input/file.xyz",
}
box (list[float]): Box dimensions xlo, ylo, zlo, xhi, yhi, zhi, in Å. If set to None
(default), pymatgen will estimate the required box size based on the volumes of
the provided molecules.
"""
Expand Down Expand Up @@ -181,37 +180,37 @@ def get_input_set(
if box:
box_list = " ".join(str(i) for i in box)
else:
# estimate the total volume of all molecules in cubic Å
# Estimate the total volume of all molecules in cubic Å
net_volume = 0.0
for d in molecules:
mol = d["coords"] if isinstance(d["coords"], Molecule) else Molecule.from_file(d["coords"])
for dct in molecules:
mol = dct["coords"] if isinstance(dct["coords"], Molecule) else Molecule.from_file(dct["coords"])

if mol is None:
raise ValueError("Molecule cannot be None.")
# pad the calculated length by an amount related to the tolerance parameter
# Pad the calculated length by an amount related to the tolerance parameter
# the amount to add was determined arbitrarily
length = (
max(np.max(mol.cart_coords[:, i]) - np.min(mol.cart_coords[:, i]) for i in range(3))
+ self.tolerance
)
net_volume += (length**3.0) * float(d["number"])
net_volume += (length**3.0) * float(dct["number"])
box_length = net_volume ** (1 / 3)
print(f"Auto determined box size is {box_length:.1f} Å per side.")
box_list = f"0.0 0.0 0.0 {box_length:.1f} {box_length:.1f} {box_length:.1f}"

for d in molecules:
for dct in molecules:
mol = None
if isinstance(d["coords"], str):
mol = Molecule.from_file(d["coords"])
elif isinstance(d["coords"], Path):
mol = Molecule.from_file(str(d["coords"]))
elif isinstance(d["coords"], Molecule):
mol = d["coords"]
if isinstance(dct["coords"], str):
mol = Molecule.from_file(dct["coords"])
elif isinstance(dct["coords"], Path):
mol = Molecule.from_file(str(dct["coords"]))
elif isinstance(dct["coords"], Molecule):
mol = dct["coords"]

if mol is None:
raise ValueError("Molecule cannot be None.")

fname = f"packmol_{d['name']}.xyz"
fname = f"packmol_{dct['name']}.xyz"
mapping[fname] = mol.to(fmt="xyz")
if " " in str(fname):
# NOTE - double quotes are deliberately used inside the f-string here, do not change
Copy link
Contributor Author

@DanielYang59 DanielYang59 Aug 6, 2024

Choose a reason for hiding this comment

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

@rkingsbury Can I get some confirmation on this change please, thanks:)

  1. I didn't see double quotes being used here, so I assume it's safe to remove this note. (The other note at line 172 is valid)
  2. I believe it's safe to remove the black tag fmt: on/off here? As I'm not seeing pre-commit formatting this file here (running black directly would indeed wrap some lines, but we're not replying on black being the formatter).

Copy link
Member

Choose a reason for hiding this comment

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

this was probably carelessly refactored by me at some point as !r results in single quotes. thanks for pointing this out. let's go back to what i think was the original state

f'structure "{fname}"\n'

Copy link
Contributor

Choose a reason for hiding this comment

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

At the time I added this, packmol was VERY pecuiliar about things like single vs. double quotes in the input file. I believe I opened an issue about it at the time, and they fixed the behavior to be a little less finicky. In principle as long as the packmol unit tests pass I think we should be OK, but it might be wise to see if someone can do some additional manual testing as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the additional input and confirmation. And ping me at anytime if anything doesn't look right, thanks!

Expand All @@ -220,7 +219,7 @@ def get_input_set(
# fmt: on
else:
file_contents += f"structure {fname}\n"
file_contents += f" number {d['number']}\n"
file_contents += f" number {dct['number']}\n"
file_contents += f" inside box {box_list}\n"
file_contents += "end structure\n\n"

Expand Down