Impr: Error message when data not found by am.load('atom_data')

[arn-all]

Another improvement to the behaviour of am.load() that I'd like to work on :

Problem

When using am.load() with an incorrectly formatted file, or when providing it a filename that doesn't exist, the error message :

UnboundLocalError: local variable 'xlo' referenced before assignment

is not the one we'd expect for a missing file.

Steps to reproduce

import atomman as am
am.load("atom_data", "hello look at this error message")

---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
<ipython-input-3-6c5913967289> in <module>
----> 1 am.load('atom_data',"hello, look at this error message")

~/.local/lib/python3.7/site-packages/atomman/load/__init__.py in load(style, input, **kwargs)
     44 
     45     elif style == 'atom_data':
---> 46         return load_atom_data(input, **kwargs)
     47 
     48     elif style == 'atom_dump':

~/.local/lib/python3.7/site-packages/atomman/load/atom_data/load.py in load(data, pbc, symbols, atom_style, units)
    106 
    107     # Create system
--> 108     box = Box(xlo=xlo, xhi=xhi,
    109               ylo=ylo, yhi=yhi,
    110               zlo=zlo, zhi=zhi,

UnboundLocalError: local variable 'xlo' referenced before assignment

Comments

• A proper verification of the parsed data is not trivial, but a first step would be at least to check that all necessary data have been assigned, and raise a ValueError exception if not.
• It would be a good idea to split the load('atom_data') function into two functions (or more !), in order to separate the data extraction and the creation of the atomman system. Tests and verification of the data integrity would become easier this way in my opinion. What do you think ?
• By the way, I saw that line 81 of this file https://github.com/lmhale99/atomman/blob/master/atomman/load/atom_data/load.py is commented ; why ? It may be a good idea to compare this number with the number of lines below, to detect errors in the reading process.
• More generally, I'm thinking of writing tests for this kind of I/O functions but it's a bit of work...

[lmhale99]

I'm for all of those suggestions.

Response to comments

• If we can somehow differentiate "missing values" from "wrong format" in throwing errors it could be a step towards automatic load format detection. Maybe ValueError for the first and a custom FileFormatError for the second? I agree that figuring out proper verification rules is not trivial.
• Separate functions for reading the different types of content (Atoms, Velocities, etc.) would make the function more adaptable for adding support of additional content types down the road.
• Getting the atypes value was commented out because I wasn't doing anything with it.
• I'm behind on tests for most parts of atomman. Maybe start with small but full file tests, then add smaller function-specific tests as the code is divided up?

[lmhale99]

I made changes to the atom_data load/dump style in preparation of an upcoming atomman 1.3.0 release.

• load now recognizes and uses the position image flags if they exist. The way I do it should be agnostic to the atom_style (count terms in Atoms table, extract image flags if 3 more than expected). dump will also output the image flags for systems with unwrapped atoms.
• The code has been divided into a few component functions now. I wasn't sure of a good way to subdivide the "first pass", but subsequent reads are separated out.
• I created a FileFormatError class and added content checks that would issue that error or a FileNotFoundError if required content was missing.
• There are a few basic pytests now for the atom_data style. More checking still needed, and cross-checking with LAMMPS as well.
• As for reading/using # atypes, I ended up changing how natypes is handled in atomman to allow symbol assignment to unused atype values. I had to do this for some potential styles and to make things like impurity defect generation easier. Because of this, the # atypes value in the data file really has no relevance to atomman and performing checks on the value would unnecessarily restrict allowed behaviors.

If you have the time, can you test the new atom_data behavior with your files to see that it is correct or if you have any other improvement suggestions?

[arn-all]

Hi,

I did different tests with my files, it all worked fine :

• different files created with the current Lammps version, write_data command. Wrapped flags work fine.
• a lammps restart binary file. Atomman said it couldn't find the number of atoms, inviting the user to check what's in his file (and eventually realise it's not a readable text file)
• a non existing filename "foo"
• Files created by the Fortran code atomsk

I just have two comments :

• If we keep ignoring the Masses section of Lammps files (documentation) it would be good to specify it somewhere in the docs (maybe it's somewhere already ?). Parsing the mass section could be a possible improvement, though I'm not sure it's of great interest as those masses could be all taken from mendeleev's table.

• The command am.load("atom_data", "") throws UnboundLocalError: local variable 'i' referenced before assignment instead of FileNotFoundError. Maybe it could be fixed in uber_open_rmode.py's __enter__.py method with something like this, but I'm not familiar with the use of context managers so it might be a stupid idea :

  file_exists = os.path.isfile(self.data)
  if file_exists:
  return True
  else:
  raise FileNotFoundError()

Anyway, THANKS for your work on that !

[lmhale99]

That's good!

• For the masses, I'll make a separate issue and try to tag you on it. The real issue isn't that atom_data ignores the masses, but that atomman can't do anything with the mass values afterwards...

• The empty string should now give a FileNotFoundError. With uber_open_rmode, it returns the given string (as a BytesIO) if the isfile(string) fails. This allows the string to be either a file name or a file's contents. The FileNotFoundError is thrown by load atom_data if the returned file-like object has only one line, which would indicate a name rather than content. This might not be perfect under all cases, but seems reasonable for most.

I'll close this one now.