mphowardlab / lammpsio

Python tools for working with LAMMPS files
BSD 3-Clause "New" or "Revised" License
7 stars 1 forks source link

Potential bug in dump schema validation #42

Closed PhilLecl closed 6 months ago

PhilLecl commented 6 months ago

There probably shouldn't be a space in the string " image":

https://github.com/mphowardlab/lammpsio/blob/d04d5dc93e14f06daa3a3e85fab80993e1c074b4/src/lammpsio/dump.py#L326

mphoward commented 6 months ago

Thanks! Almost definitely not, but do you have an example where this caused an issue that we can use to make a test? It’s OK if you don’t, as I think we should go ahead with this 1 line fix regardless.

PhilLecl commented 6 months ago

It didn't cause any errors for me - I think this only has an effect with faulty dump files, in which case a different error message than intended is shown.
Here's an example I've quickly put together. Consider the following Python code:

import lammpsio
traj = lammpsio.DumpFile('dump.foo')
for snap in traj:
    pass

If run using the following dump with a missing z column:

ITEM: TIMESTEP
0
ITEM: NUMBER OF ATOMS
1
ITEM: BOX BOUNDS pp pp pp
-7.0 7.0
-7.0 7.0
-7.0 7.0
ITEM: ATOMS id type x y
1 1 0 0

The following error occurs, as intended: OSError: lammpsio requires 3-element vectors

If run using the following dump with a missing iz column, however:

ITEM: TIMESTEP
0
ITEM: NUMBER OF ATOMS
1
ITEM: BOX BOUNDS pp pp pp
-7.0 7.0
-7.0 7.0
-7.0 7.0
ITEM: ATOMS id type ix iy
1 1 0 0

This error occurs: TypeError: list indices must be integers or slices, not NoneType. If the space in " image" is removed, the intended error message is shown.

mphoward commented 6 months ago

Awesome, makes sense to me! We will fix this and also add a test for it based on your reproducer. I don't think we have any tests for ill-formed dump files currently.