Repository navigation
IMDv2 generic support implementation with tests - #141
amruthesht wants to merge 38 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extends imdclient to handle more IMDv2 streaming patterns (notably variable energy-packet ordering) and adds/updates integration tests and simulation inputs to validate IMDv2 behavior across supported engines.
Changes:
- Update IMDv2 frame parsing to tolerate multiple leading energy packets before coordinates and to reuse cached energies when energies are omitted.
- Add IMDv2 integration test coverage alongside existing IMDv3 tests for NAMD, GROMACS, and LAMMPS.
- Add new IMDv2 engine input/config files and rename/refactor test datafile constants to separate v2/v3 variants.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| imdclient/IMDClient.py | Adjust IMDv2 producer parsing to handle multiple/optional energy packets before coordinates. |
| imdclient/tests/base.py | Refactor integration test base class and add an IMDv2-specific integration test class. |
| imdclient/tests/test_namd.py | Split shared NAMD fixtures and add IMDv2 + IMDv3 integration test classes with version-specific inputs. |
| imdclient/tests/test_lammps.py | Split shared LAMMPS fixtures and add IMDv2 + IMDv3 integration test classes with version-specific inputs. |
| imdclient/tests/test_gromacs.py | Split shared GROMACS fixtures and add IMDv2 + IMDv3 integration test classes with version-specific inputs. |
| imdclient/tests/minimalreader.py | Handle missing box data when converting to MDAnalysis dimensions during stream processing. |
| imdclient/tests/datafiles.py | Rename/split datafile constants to explicitly distinguish v2 vs v3 simulation inputs. |
| imdclient/data/namd/md/namd_v2_nst_1.namd | Add NAMD IMDv2 test configuration (NST=1). |
| imdclient/data/namd/md/namd_v2_nst_8.namd | Add NAMD IMDv2 test configuration (NST=8). |
| imdclient/data/lammps/md/lammps_v2_nst_1.in | Add LAMMPS IMDv2 test input (NST=1). |
| imdclient/data/lammps/md/lammps_v2_nst_8.in | Add LAMMPS IMDv2 test input (NST=8). |
| imdclient/data/lammps/md/lammps_v3_nst_1.in | Expand LAMMPS IMDv3 fix options to explicitly enable time/box/coords/vel/forces. |
| imdclient/data/lammps/md/lammps_v3_nst_8.in | Expand LAMMPS IMDv3 fix options to explicitly enable time/box/coords/vel/forces. |
| imdclient/data/gromacs/md/gromacs_v2_nst1.mdp | Add GROMACS IMDv2 test MDP (NST=1). |
| imdclient/data/gromacs/md/gromacs_v2_nst8.mdp | Add GROMACS IMDv2 test MDP (NST=8). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fca1842 to
bd67a88
Compare
orbeckst
left a comment
There was a problem hiding this comment.
As discussed in meeting, wrap the GROMACS streamed coordinates into the unit cell by
- extracting the ts.dimensions from the saved trajectory
- create two on-the-fly transformations
- use the transformations with the in-memory universe that you build with the numpy array of the IMD data
This should generate an in-memory trajectory of wrapped coordinates.
orbeckst
left a comment
There was a problem hiding this comment.
I only managed a partial review (code, not tests). The fact that it works is great!
Do we use the the same IMDClient for v3 and v2? Do any of the changes affect v3?
My primary comments so far are just all related to docs.
We need to document the v2 capabilities. Can you create a protocol page for the v2 format as we understand it (then we have a v2 and a v3 page). On the IMDClient docs page we should also have a section on v2 that lists any shortcomings; some out of our control (no dimensions, no v/f), some our implementation (energies can be out of sync as I understand it).
Add an entry to CHANGELOG. This will have to be 0.3.0.
| IMD transmission rate to be set after client send go signal. | ||
| This parameter is only set to the server when using IMDv2. Default behavior is to not set the transmission rate. | ||
| This parameter is useful when running GROMACS with IMDv2, where transmission rate can only be set via the client. | ||
| IMDv2 implementations in LAMMPS and NAMD support setting the transmission rate via relevant input file parameters. |
There was a problem hiding this comment.
- Add an definition of what "IMD transmission rate" is, e.g., a which integrator step IMD packages are sent. Include a link to https://imdclient.readthedocs.io/en/latest/protocol_v3.html#transmission-rate
IMD :ref:`transmission rate <transmission-rate>` to be set after client send go signal.- State what the default value for IMDv2 in GROMACS is.
There was a problem hiding this comment.
Does GROMACS default to transmission_rate = 1? I.e., is it safe for users to leave it as None or will it hang their simulations?
You could add a note box
.. warning::
When using GROMACS with IMDv2, users should set `transmission_rate` explicitly.
Otherwise the GROMACS default of 1 will be chosen. This default setting will send
every integrator time step and may impact performance substantially.There was a problem hiding this comment.
And add an indication in which version this option was added
transmission_rate : int, optional [``None``]
IMD transmission rate to be set after client send go signal.
This parameter is only set to the server when using IMDv2. Defaul...
...
IMDv2 implementations in LAMMPS and NAMD support setting the transmission rate via relevant input file parameters.
.. versionadded:: 0.3.0Check that it looks right when the docs are rendered.
There was a problem hiding this comment.
Modified and added based on IMDv2 behavior in GROMACS, LAMMPS, and NAMD
| This parameter is useful when running GROMACS with IMDv2, where transmission rate can only be set via the client. | ||
| IMDv2 implementations in LAMMPS and NAMD support setting the transmission rate via relevant input file parameters. | ||
| **kwargs : dict (optional) | ||
| Additional keyword arguments to pass to the :class:`BaseIMDProducer` and :class:`IMDFrameBuffer` |
There was a problem hiding this comment.
Add a
.. versionchanged:: 0.3.0
Added capability to process IMDv2 protocol streams.(Note: need two blank lines above)
| header = self._get_header() | ||
| if header.type == IMDHeaderType.IMD_ENERGIES and header.length == 1: | ||
| leading_energies = 0 | ||
| while header.type == IMDHeaderType.IMD_ENERGIES and header.length == 1: |
There was a problem hiding this comment.
When do you get multiple energy packets in sequence? Only in v2?
Add a note that energy handling in v2 can miss something/report out of sync.
| # Either receive energies + positions or just positions | ||
| # Consume any leading energy packets first, then handle positions. |
There was a problem hiding this comment.
Does this change how v3 streams are processed?
There was a problem hiding this comment.
No, this is only implemented under IMDProducerV2
|
|
||
| self._go() | ||
|
|
||
| if transmission_rate is not None and self._imdsinfo.version == 2: |
There was a problem hiding this comment.
If version == 3 and transmission rate is set, issue a warning that the kwarg is ignored, for least surprise to user
There was a problem hiding this comment.
(I know the docs already say this, so if you think that's sufficient, feel free to ignore , just thinking of principle of least surprise)
|
I released 0.2.4 #146 so please create the 0.3.0 section in the CHANGELOG when you add your entry. |
| @pytest.fixture() | ||
| def first_frame(self): | ||
| return 0 | ||
| def imd_nst(self, mdp): |
| return request.param | ||
|
|
||
| @pytest.fixture() | ||
| def first_frame(self, inp): |
| return request.param | ||
|
|
||
| @pytest.fixture() | ||
| def first_frame(self, inp): |
| # def match_string(self): | ||
| # return "IMD: Will wait until I have a connection and IMD_GO orders." | ||
|
|
||
| class TestIMDv3Gromacs(IMDGromacsTest, IMDv3IntegrationTest): |
| return request.param | ||
|
|
||
|
|
||
| class TestIMDv2Gromacs(IMDGromacsTest, IMDv2IntegrationTest): |
| yield u | ||
|
|
||
|
|
||
| class TestIMDv3Lammps(IMDLammpsTest, IMDv3IntegrationTest): |
| return 0 | ||
|
|
||
|
|
||
| class TestIMDv2Lammps(IMDLammpsTest, IMDv2IntegrationTest): |
| return 0 | ||
|
|
||
|
|
||
| class TestIMDv3NAMD(IMDNAMDTest, IMDv3IntegrationTest): |
| ) | ||
|
|
||
|
|
||
| class TestIMDv2NAMD(IMDNAMDTest, IMDv2IntegrationTest): |
- added ValueError raising in `__init__` - tests to check error - updated API doc
|
Can you please update to sync it with the base branch? Thanks! Will try to look at it over the weekend. |
orbeckst
left a comment
There was a problem hiding this comment.
Add entry to CHANGELOG and write somewhere in the docs explicitly that we can also work with v2. Mention why this is not great for analysis.
| gen_vel = no | ||
| IMD-group = System | ||
| IMD-nst = 8 | ||
| IMD-version = 2 |
There was a problem hiding this comment.
This will only work with our patched version of GROMACS that also speaks v3. I think you should leave it out.
There was a problem hiding this comment.
Added new docker image support for vanilla gromacs versions as a PR. once mereged these tests would tests imdv2 support for imdclient using appropriate version of all 3 MD engines (GROMACS, LAMMPS and NAMD)
|
As discussed: use a vanilla version of GROMACS (eg 2025 from conda-forge) to test v2, too. |
|
The actual MD engine tests are failing, eg https://github.com/Becksteinlab/imdclient/actions/runs/37972189712/job/113961342160?pr=141 with What's the issue? |
|
From discussion: first need new docker images Becksteinlab/streaming-md-docker#32 |
|
We have new images (I fixed the deployment workflow, see also Becksteinlab/streaming-md-docker#34 ). If this works here now, then we should be good. |
|
@amruthesht the MD engine tests fail with multiple Connection to localhost:...... refused errors. This occurs for GROMACS, NAMD, and LAMMPS. Eg all GROMACS tests failed and LAMMPS and NAMD |
This PR provides generic IMDv2 support for handling various v2 streaming scenarios in
imdclientFixes #140 , #143
Changes made in this Pull Request:
IMD-unwrap=Truein session infotratesending viaIMDClientobjectNote: IMDv2 in GROMACS seems to expects transmission rate from the user via the client, as discussed in #143
It ignores
IMD-nstin the*.mdpfile. Thus theIMDClientobject was modified to sendIMD_TRATEright afterIMD_GOas set in the object initiation when using v2.PR Checklist