IMDReader removed from imdclient

[amruthesht]

Fixes #53

Changes made in this Pull Request:

• package only contains imdclient
• IMDReader and StreamReaderBase has been removed from the main package
• Tests for the IMDReader have been removed from the tests folder
• package does not require MDAnalysis (except for testing)

Related development

• The IMDReader is being added to MDAnalysis in PR feature: IMDReader Integration MDAnalysis/mdanalysis#4923 (see issue Adopt a reader for live-streamed Interactive MD (IMD) version 3 trajectories MDAnalysis/mdanalysis#4827 for broader discussion)
• See issue Make the AnalysisBase compatible with live simulation streams MDAnalysis/mdanalysis#4828 on what to do with StreamReaderBase.

PR Checklist

• Tests?
• Docs?
• CHANGELOG updated?
• Issue raised/referenced?

[amruthesht]

TO-DOs

Get rid of any usage or references to MDAnalysis (as suggested by @orbeckst)

• Remove any MDAnalysis usage for generic tests
• Make simple pseudo "reader" that can be used for tests

[orbeckst]

Remove MDAnalysis from pyproject.toml etc — there shouldn't be a any place in the package where it gets installed or imported.

For right now, streamanalysis is in a weird place. We can leave it in there and test it or we can disable/remove it. Something to discuss.

[orbeckst]

As discussed, we can use MDAnalysis for testing (reading trajectories) but not for streaming itself.

[orbeckst]

My primary issue with the current approach is that it still requires IMDReader for testing. The way that we had concluded our discussion (ASU+MDA meeting on 2025-03-03) was the decision

imdclient: allow MDA as test dependency for imdclient tests (but do not allow IMDReader)

In my comment below I am outlining how you can use MDAnalysis to construct a Universe after obtaining the data from the stream as numpy arrays. For this to work you need to write code that only uses imdclient to pull data from a stream.

[orbeckst]

The minimalReader is progress and now you have to use it in the tests. That's the next big step.

Let's keep the the parse ports function in imdclient.

[orbeckst]

@amruthesht do you need additional input to make progress here?

[orbeckst]

In particular, @ljwoods2 's suggestion from #54 (comment) to just extract the frames in the most simple manner (without even needing to write a class) seems like a good way forward:

def extract_all_frames(imdclient, topology_file):
    imd_pos = []
    imd_vel = []
    imd_force = []
    while True:
        try:
            frame = imdclient.get_frame()
            imd_pos.append(frame.positions.copy())
            ...
        except EOFError as e:
            break
     # instantiate in memory universe
     # If imd_pos has shape (n_frames, n_atoms, 3) then we should be able to
     # get an in-memory universe by just passing the array:
     u = mda.Universe(topology_file, np.array(imd_pos))
     return u

Todo: figure out how to add velocities and forces to the in-memory universe.

[orbeckst]

In principle the minimalReader approach seems sensible but does need some refinements.

The MD engine tests are currently failing so this needs to be fixed.

We also currently don't show code coverage so it's not clear that the tests cover everything. Codecoverage checks are something we need to add but in another PR.

[ljwoods2]

Is the goal to use minimalReader to ultimately make the trajectory available to write into TRR? Or to keep it in memory in numpy arrays and compare those directly? You may run into some trouble trying to write from the minimalReader object into TRR since the writer API requires a Universe or AtomGroup

https://docs.mdanalysis.org/stable/documentation_pages/coordinates/base.html#MDAnalysis.coordinates.base.WriterBase.write

[codecov]

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 22.90%. Comparing base (cba91fc) to head (2eb3425).
Report is 1 commits behind head on main.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[orbeckst]

Something is wrong in the NAMD tests where you're still trying to build a Universe directly from a stream instead of using minimalReader.

[orbeckst]

@hmacdope @jaclark5 @yuxuanzhuang if one of you could also have a look here to help get imdclient updated soon? Thanks.

[hmacdope]

@jaclark5 @yuxuanzhuang and myself will all review @orbeckst :)

[orbeckst]

@ljwoods2 can you please use approve/request changes for reviews? I am just looking at my own requested changes and will likely approve as soon as they are addressed and leave it to you to assess if your comments have been addressed. Thanks!

[orbeckst]

Only a few things todo from my end, mainly

• correctly list dependencies in pyproject.toml
• restructuring and rewriting some of the docs to clearly separate IMDv3 specs from implementation in codes and also document IMDv2.

[orbeckst]

Not sure if coverage is correctly reported but this may be something to figure out once this is merged.

[orbeckst]

@amruthesht why did we remove this information?

I thought we wanted to keep it as documentation of the existing protocol. Unless there was a decision to remove, please put it back in.

[orbeckst]

We can reference it as

Although the full IMDv2 protocol is not officially documented anywhere, one can infer it from the examples on the VMD/NAMD pages https://www.ks.uiuc.edu/Research/vmd/imd/ or the implementations in MD packages such as GROMACS. Here we summarize our understanding of IMDv2 as inferred from the GROMACS implementation.

[ljwoods2]

This page should be removed from the public-facing docs until we can rewrite it but I agree, no reason to fully delete it. I wrote it a long time ago and I think our understanding of v2 has improved since then. It's also not formatted well. I'd like to remove it for now, since as you said it can be inferred from above sources, plus we've fully documented v2 as a "diff" in the v3 page with the "Present in IMDv2" table and with notes like this (for example):

"Changed in version 3: In IMDv2, pause acted as a toggle, meaning sending a pause packet twice would pause and then resume the simulation’s execution. In IMDv3, the resume packet is required to resume a paused simulation since pausing is idempotent."

[orbeckst]

Thanks for weighing in @ljwoods2.

Let's keep the page around for now but don't link it (ignore sphinx's complaints). The docs will need a bit more work but we can do this separately.

[orbeckst]

I am not sure if the other functions are properly tested. We need to look at coverage after this PR is merged.

[orbeckst]

@amruthesht do you think we'll be able to merge the PR by tomorrow Friday end of day?

[ljwoods2]

LGTM pending @orbeckst's documentation and pyproject.toml requested changes being completed. Approving now so I don't block tomorrow

[yuxuanzhuang]

lgtm! Just couple of unblocking documentation comments

[orbeckst]

@amruthesht please also add yourself to AUTHORS.

Please ping me when you need a review.

[amruthesht]

Thank you @orbeckst, @ljwoods2, @yuxuanzhuang and @jaclark5 for your comments!

@orbeckst - I have addressed all outstanding issues, comments and suggestions raised by everyone. We still need to look at coverage and make sure all functions have tests (maybe after this PR is merged), as you pointed out.
I think it is ready for a final review and merge!

[orbeckst]

Looks good!

(I'll fix a small thing and we will need to look at testing/coverage #75 and docs #76 but let's move forward!)

Well done!