Skip to content

Commit

Permalink
fix assertions for file IO
Browse files Browse the repository at this point in the history
  • Loading branch information
davkovacs authored Sep 30, 2022
1 parent a080a55 commit d569918
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions mace/data/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,11 @@ def load_from_xyz(
charges_key: str = "charges",
extract_atomic_energies: bool = False,
) -> Tuple[Dict[int, float], Configurations]:
assert file_path[-4:] == ".xyz", NameError("Specify file with extension .xyz")

atoms_list = ase.io.read(file_path, format="extxyz", index=":")
try:
atoms_list = ase.io.read(file_path, index=":")
except:
raise Exception("Invalid file format for {}".format(file_path))

This comment has been minimized.

Copy link
@bernstei

bernstei Sep 30, 2022

Collaborator

Is there a good reason to catch here? There are other possible errors like file not found, which are going to be obscured by this message. I'd suggest just a plain read (not in a try/except), and letting any exception just happen and get dealt with outside this function.

If for some reason we do want to catch, it's slightly cleaner if we do

except Exception as exc:
    raise RuntimeError(f"Invalid file format for {file_path}") from exc

or maybe ValueError.

We could also specifically catch ase.io.formats.UnknownFileTypeError, which is what's actually raised for a bad file type, if we want to deal with file type errors differently from other errors.

This comment has been minimized.

Copy link
@davkovacs

davkovacs Oct 2, 2022

Author Collaborator

I see what you mean. Probably best to let ASE throw an error as you recommend, I am fixing it accordingly.

if not isinstance(atoms_list, list):
atoms_list = [atoms_list]

Expand Down

0 comments on commit d569918

Please sign in to comment.