-
Notifications
You must be signed in to change notification settings - Fork 871
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
Fix Atoms.cluster_from_file()
in io.feff.inputs
giving wrong number of atoms
#3426
Conversation
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
This reverts commit 8112068.
…checks initialize PymatgenTest.TEST_STRUCTURES as dict[Path, None]
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
pymatgen/io/feff/inputs.py
Outdated
coords = [] | ||
symbols = [] | ||
for line in lines: | ||
if line: | ||
if line and line[0].__contains__("*"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if line and line[0].__contains__("*"): | |
if line and line.startswith("*"): |
tests/io/feff/test_inputs.py
Outdated
def test_atom_num(self): | ||
atoms = Atoms.cluster_from_file("ATOMS_test.inp") | ||
num_atom = 37 | ||
assert len(atoms) != num_atom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not test for the expected number of atoms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Janosh,
The expected number of atoms is 37; how should I specify this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is !=
a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing one level of indentation to make it a method of the test class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me fix it. sorry to confuse you. I want it to pass when the expected number of atoms is equal to num_atom and throw an error when the numbers are not equal.
tests/files/ATOMS_test.inp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have 6 or more existing .inp
files. Can you reuse those for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
tests/io/feff/test_inputs.py
Outdated
def test_atom_num(self): | ||
atoms = Atoms.cluster_from_file("ATOMS_test.inp") | ||
num_atom = 37 | ||
assert len(atoms) != num_atom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing one level of indentation to make it a method of the test class.
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks almost done but can you fix the failing tests: FileNotFoundError: [Errno 2] No such file or directory: 'ATOMS_test.inp'
?
Signed-off-by: KaifengZheng <[email protected]>
Signed-off-by: KaifengZheng <[email protected]>
for line in lines: > if line and not line.startswith("*"):
Atoms.cluster_from_file()
in io.feff.inputs
giving wrong number of atoms
Summary
Atoms.cluster_from_file() generates a cluster with a wrong number of atoms because it skips wrong lines when the format of feff input file in the Atom parts is different.
Major changes:
Checklist
ruff
.mypy
.duecredit
@due.dcite
decorators to reference relevant papers by DOI (example)Tip: Install
pre-commit
hooks to auto-check types and linting before every commit: