feature: IMDReader Integration #4923

amruthesht · 2025-02-19T22:47:49Z

This draft PR addresses the feature request discussed in #4827.

Note:
The IMDReader feature which was previously a part of the imdclient package has been moved into MDAnalysis below. Any other modules have been in retained in imdclient, which has been added as an optional dependency here. We are currently in the process of splitting the imdclient package as mentioned above. (Issue, PR)

Major changes made in this Pull Request:

IMDReader, other associated base classes and a utility function were added to coordinates in the main package.
Appropriate test cases for the reader were added as a part of test-imd.py
Other corresponding changes were made to CI and environment *.yaml files
imdclient was added as an optional dependency

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

📚 Documentation preview 📚: https://mdanalysis--4923.org.readthedocs.build/en/4923/

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2025-02-19T22:58:47Z

Codecov Report

Attention: Patch coverage is 32.33533% with 113 lines in your changes missing coverage. Please review.

Project coverage is 85.28%. Comparing base (b79c77d) to head (cf15cf9).
Report is 28 commits behind head on develop.

Files with missing lines	Patch %	Lines
package/MDAnalysis/coordinates/IMD.py	35.22%	56 Missing and 1 partial ⚠️
package/MDAnalysis/coordinates/base.py	28.20%	56 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (b79c77d) and HEAD (cf15cf9). Click for more details.

HEAD has 5 uploads less than BASE

Flag BASE (b79c77d) HEAD (cf15cf9)

7 2

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4923      +/-   ##
===========================================
- Coverage    93.66%   85.28%   -8.38%     
===========================================
  Files          177      178       +1     
  Lines        21850    22168     +318     
  Branches      3079     3137      +58     
===========================================
- Hits         20466    18907    -1559     
- Misses         933     2792    +1859     
- Partials       451      469      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

amruthesht · 2025-02-19T23:02:30Z

Your thoughts on this are appreciated - @orbeckst

orbeckst

Thanks for the initial PR.

The first big step is to get the tests running properly so that the CI uses an imdclient without IMDReader. Otherwise we are not sure we're testing the code here.
Minor initial comments while I skimmed.
Simple thing: run black over all files to get the formatting and ordering of imports right

orbeckst · 2025-02-19T23:16:28Z

.github/actions/setup-deps/action.yaml

+  imdclient:
+    default: 'imdclient'


It's going to take time to get imdclient to a stage where the imdclient package does not actually affect MDAnalysis.

Is there a way that we could temporarily (for initial CI testing) install imdclient from a branch or tarball, e.g., in a pip section? Then we could fairly rapidly create a preliminary (unpublished) imdclient package without IMDReader.

By initial CI testing, do you mean "in this PR"?

There's a pip section just below, which should work if you put in the git location for pip install, but also you can just temporarily modify the CI script to do an additional pip install if it's for "testing within the PR itself".

If it's "after merge", this would require more discussion.

Just for right now to bootstrap the PR.

I don't want to merge without a solid conda-forge imdclient package in place.

package/MDAnalysis/coordinates/IMD.py

package/MDAnalysis/coordinates/base.py

orbeckst · 2025-02-19T23:21:32Z

package/MDAnalysis/coordinates/util.py

Just put this code to where it's needed, e.g., directly into IMD.py.

Do not add a util.py here, we need to keep this module as clean as possible because it's already quite crowded.

package/MDAnalysis/coordinates/util.py

orbeckst · 2025-02-19T23:22:16Z

package/pyproject.toml

@@ -79,6 +79,7 @@ extra_formats = [
    "pytng>=0.2.3",
    "gsd>3.0.0",
    "rdkit>=2020.03.1",
+    "imdclient",


We almost certainly need to add a minimal version.

testsuite/MDAnalysisTests/coordinates/test_imd.py

IAlibay · 2025-02-19T23:37:02Z

package/MDAnalysis/coordinates/IMD.py

+logger = logging.getLogger("imdclient.IMDClient")
+
+
+class IMDReader(StreamReaderBase):


I think I'm getting confused about which PR is for which thing. @orbeckst given our discussion earlier this week, and your comment above which I take to be "IMDClient is still in flux", does it not make sense for the IMDReader to exist upstream and then just import it here? (edit: here my intent is "well then you could make releases and it wouldn't be limited to MDA release frequency").

We have to split IMDReader from imdclient and make a version of imdclient without IMDReader (which is in the works Becksteinlab/imdclient#54 ). At the same time we are moving what was split off into coordinates.IMD.

Amru is working on both at the moment.

The way IMDReader depends on imdclient is not the problem, and imdclient itself is also pretty stable, it's just that the tests for imdclient have made use of a lot of MDAnalysis/IMDReader for convenience, and we now have to rewrite some of these tests to use bare-bones python.

orbeckst · 2025-02-19T23:50:31Z

I set the PR to Work in progress for the time being, just to indicate that we're not yet at the stage where the CI is working. Once the tests run properly, we can update the status.

Obviously, this shouldn't discourage anyone from contributing and commenting.

IAlibay

A first quick look

testsuite/MDAnalysisTests/coordinates/test_imd.py

jaclark5

I read through and nothing jumped out at me that wasn't already mentioned, except that I didn't see these changes in the documentation that was linked. The following needs to be added:

Add doc/sphinx/source/documentation_pages/coordinates/IMD.rst
.. automodule:: MDAnalysis.coordinates.IMD

doc/sphinx/source/documentation_pages/coordinate_modules.rst
coordinates/IMD

doc/sphinx/source/documentation_pages/references.rst
If you use IMD capability...

hmacdope

Sorry for the delay, had a lookover, I will try and push some changes addressing some of these myself also, but would be good to pick up the momentum here again if possible.

hmacdope · 2025-03-22T23:52:30Z

package/MDAnalysis/coordinates/IMD.py

+
+try:
+    import imdclient
+    from imdclient.IMDClient import IMDClient


These need test coverage with mocks

Sorry @hmacdope, I'm not entirely sure what you mean here, could you please elaborate, thank you!

@hmacdope do you have a moment to add some pointers for mocks, e.g., examples in the current tests that you have in mind?

You can get coverage on the import guards by mocking the import using something like this. This was taken verbaitm from a PR of mine but the general idea should carry over nicely. Doesn't use mock directly but still effective.

def test_HAS_DISTOPIA_distopia_too_old(): # mock a version of distopia that is too old sys.modules.pop("distopia", None) sys.modules.pop("MDAnalysis.lib._distopia", None) module_name = "distopia" mocked_module = ModuleType(module_name) # too old version mocked_module.__version__ = "0.1.0" sys.modules[module_name] = mocked_module import MDAnalysis.lib._distopia assert HAS_DISTOPIA

package/MDAnalysis/coordinates/IMD.py

package/MDAnalysis/coordinates/base.py

testsuite/MDAnalysisTests/coordinates/test_imd.py

hmacdope · 2025-03-27T09:55:18Z

@amruthesht do we have a plan for pushing ahead with suggested changes?

amruthesht · 2025-03-27T15:33:14Z

@amruthesht do we have a plan for pushing ahead with suggested changes?

Working on the imdclient repository right now. I had a few small issues there with using the IMDReader, but getting them fixed soon.
And then I'll wrap up some small changes on the mdanalysis end and we should be good to go!

1. Moved `parse_host_port` to `IMD.py` and deleted `util.py` 2. Cleaned up `test_imd.py` - changes to `assert_*` functions and simplified non-applicable test to pass automatically

hmacdope · 2025-05-05T15:11:49Z

@amruthesht kicking CI here.

hmacdope · 2025-05-05T15:12:04Z

@yuxuanzhuang would you be able to have a run through?

yuxuanzhuang · 2025-05-05T16:04:55Z

I will have another look at the PR this week. One immediate thing is to add documentation.

Please refer to the comments below.

I read through and nothing jumped out at me that wasn't already mentioned, except that I didn't see these changes in the documentation that was linked. The following needs to be added:

Add doc/sphinx/source/documentation_pages/coordinates/IMD.rst .. automodule:: MDAnalysis.coordinates.IMD

doc/sphinx/source/documentation_pages/coordinate_modules.rst coordinates/IMD

doc/sphinx/source/documentation_pages/references.rst If you use IMD capability...

hmacdope · 2025-05-05T19:33:43Z

Need to figure out tests also @amruthesht

amruthesht added 2 commits February 19, 2025 08:23

IMDReader integration

a6f2bb2

small cleanup

a67cbfb

github-actions bot reviewed Feb 19, 2025

View reviewed changes

orbeckst requested changes Feb 19, 2025

View reviewed changes

IAlibay reviewed Feb 19, 2025

View reviewed changes

orbeckst marked this pull request as draft February 19, 2025 23:48

orbeckst assigned hmacdope and yuxuanzhuang Feb 19, 2025

IAlibay requested changes Feb 20, 2025

View reviewed changes

jaclark5 requested changes Feb 23, 2025

View reviewed changes

hmacdope requested changes Mar 23, 2025

View reviewed changes

amruthesht and others added 2 commits April 9, 2025 17:12

Chore: Cleanup and small fixes

8c00bc1

1. Moved `parse_host_port` to `IMD.py` and deleted `util.py` 2. Cleaned up `test_imd.py` - changes to `assert_*` functions and simplified non-applicable test to pass automatically

bump CI

bc88b7b

hmacdope requested a review from yuxuanzhuang May 5, 2025 15:12

Add minimum docs

cf15cf9

jaclark5 self-requested a review May 24, 2025 23:52

		logger = logging.getLogger("imdclient.IMDClient")


		class IMDReader(StreamReaderBase):

feature: IMDReader Integration #4923

Are you sure you want to change the base?

feature: IMDReader Integration #4923

Uh oh!

Conversation

amruthesht commented Feb 19, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Developers Certificate of Origin

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amruthesht commented Feb 19, 2025

Uh oh!

orbeckst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IAlibay Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Feb 19, 2025

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaclark5 left a comment

Choose a reason for hiding this comment

Uh oh!

hmacdope left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmacdope commented Mar 27, 2025

Uh oh!

amruthesht commented Mar 27, 2025

amruthesht commented Feb 19, 2025 •

edited by github-actions bot

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

orbeckst left a comment •

edited

Loading

IAlibay Feb 19, 2025 •

edited

Loading