Add LAMMPS testing #78
Conversation
…icles are in box.
|
@mayukh33, could you review the changes to |
Looks good to me! |
|
@mphoward, this is ready for your review! |
| @@ -42,3 +42,51 @@ jobs: | |||
| - name: Test (optin) | |||
| run: | | |||
| python -m pytest | |||
| test-lammps: | |||
There was a problem hiding this comment.
These don't seem like they are separate tests, since you are essentially rerunning everything in the test step here. Either you should mark the tests that need LAMMPS and only run those here:
https://docs.pytest.org/en/stable/example/markers.html
or else this test step can basically just replace the one above.
| assert numpy.allclose(snap_2.charge, 0) | ||
| else: | ||
| assert not snap_2.has_charge() | ||
| _tmp.cleanup() |
There was a problem hiding this comment.
| _tmp.cleanup() |
| @@ -1,8 +1,18 @@ | |||
| import os | |||
| import tempfile | |||
There was a problem hiding this comment.
| import tempfile |
| def test_data_file_min_lammps(snap, atom_style, shuffle_ids): | ||
| if shuffle_ids: | ||
| snap.id = [3, 1, 2] | ||
|
|
||
| # write the data file with default values using lammpsio | ||
| _tmp = tempfile.TemporaryDirectory() | ||
| directory = _tmp.name | ||
| filename = directory + "/atoms.data" |
There was a problem hiding this comment.
Use the tmp_path fixture rather than creating your own:
https://docs.pytest.org/en/6.2.x/tmpdir.html
| def test_data_file_min_lammps(snap, atom_style, shuffle_ids): | |
| if shuffle_ids: | |
| snap.id = [3, 1, 2] | |
| # write the data file with default values using lammpsio | |
| _tmp = tempfile.TemporaryDirectory() | |
| directory = _tmp.name | |
| filename = directory + "/atoms.data" | |
| def test_data_file_min_lammps(tmp_path, snap, atom_style, shuffle_ids): | |
| if shuffle_ids: | |
| snap.id = [3, 1, 2] | |
| # write the data file with default values using lammpsio | |
| filename = tmp_path / "atoms.data" |
You may need one additionally command to stringify the file path.
| # create file with 2 snapshots | ||
| schema = { | ||
| "id": 13, | ||
| "position": (11, 10, 12), | ||
| "image": (9, 8, 7), | ||
| "velocity": (4, 5, 6), | ||
| "typeid": 3, | ||
| "mass": 2, | ||
| "molecule": 1, | ||
| "charge": 0, | ||
| } |
There was a problem hiding this comment.
You should probably move this schema closer to where it is used. However, is this even necessary, or can the schema just be deduced from the file now?
| f = lammpsio.DumpFile.create(filename, schema, snaps) | ||
| assert filename.exists | ||
| assert len(f) == 2 |
There was a problem hiding this comment.
Isn't this overwriting the file that lammps created, destroying the test?
| f = lammpsio.DumpFile.create(filename, schema, snaps) | |
| assert filename.exists | |
| assert len(f) == 2 |
| f2 = lammpsio.DumpFile(filename, sort_ids=sort_ids) | ||
| read_snaps = [s for s in f2] | ||
| for i, s in enumerate(f2): | ||
| assert read_snaps[i].N == snaps[i].N |
There was a problem hiding this comment.
| f2 = lammpsio.DumpFile(filename, sort_ids=sort_ids) | |
| read_snaps = [s for s in f2] | |
| for i, s in enumerate(f2): | |
| assert read_snaps[i].N == snaps[i].N | |
| f = lammpsio.DumpFile(filename, sort_ids=sort_ids) | |
| assert filename.exists | |
| assert len(f) == 2 | |
| for read_snap, snap in zip(f, snaps): | |
| assert read_snap.N == snap.N |
|
|
||
|
|
||
| @pytest.mark.skipif(not has_lammps, reason="lammps not installed") | ||
| def test_copy_from_lammps(snap, tmp_path): |
There was a problem hiding this comment.
Is this test necessary? Is the purpose only to test the copy_from argument, or is it doing something else? If only copy_from, that is a purely Python thing so I don't think we really need to test that with LAMMPS.
|
|
||
|
|
||
| @pytest.mark.skipif(not has_lammps, reason="lammps not installed") | ||
| def test_copy_from_topology_lammps(snap_8, tmp_path): |
There was a problem hiding this comment.
Same comment here. If we've verified the topology info passes through LAMMPS correctly for the DataFile, I don't see a reason to separately test the copy_from feature on its again. If the test is doing something in addition to that, please clarify!
No description provided.