Skip to content

Conversation

@amruthesht
Copy link

@amruthesht amruthesht commented Oct 10, 2025

The following PR address Issue #427 and adds UserGuide documentation for the IMDReader via imd.rst and a tutorial/example simulation_imd.ipynb.

The template for the documentation followed is deatiled in the original Issue #427


📚 Documentation preview 📚: https://mdanalysisuserguide--430.org.readthedocs.build/en/430/

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive UserGuide documentation for the IMDReader class, enabling users to perform real-time streaming analysis of molecular dynamics simulations using the Interactive Molecular Dynamics (IMD) v3 protocol. The documentation follows the template specified in Issue #427.

  • Adds complete documentation for IMD streaming including setup, usage, and limitations
  • Provides a comprehensive tutorial notebook with practical examples for different MD engines
  • Integrates the new documentation into the existing documentation structure

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
doc/source/formats/reference/imd.rst Complete IMD format documentation with setup instructions, usage examples, and API reference
doc/source/examples/other/streaming_imd.ipynb Tutorial notebook demonstrating real-time streaming analysis with practical examples
doc/source/examples/other/README.rst Updated to include the new streaming_imd tutorial in the documentation index

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"### GROMACS Setup\n",
"\n",
"Add these comprehensive IMD settings to your `.mdp` file:\n",
"```code\n",
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code block language identifier should be a valid language (e.g., 'bash', 'text', or 'ini') instead of 'code' for proper syntax highlighting in the documentation.

Copilot uses AI. Check for mistakes.
"### LAMMPS Setup\n",
"\n",
"Use the comprehensive IMD fix in your input script:\n",
"```code\n",
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code block language identifier should be a valid language (e.g., 'bash', 'text', or 'lammps') instead of 'code' for proper syntax highlighting in the documentation.

Suggested change
"```code\n",
"```lammps\n",

Copilot uses AI. Check for mistakes.
"### NAMD Setup\n",
"\n",
"Add comprehensive IMD configuration to your NAMD configuration file:\n",
"```code\n",
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code block language identifier should be a valid language (e.g., 'bash', 'text', or 'tcl') instead of 'code' for proper syntax highlighting in the documentation.

Copilot uses AI. Check for mistakes.
@orbeckst
Copy link
Member

@ljwoods2 could you also have a quick look please?

@orbeckst orbeckst self-assigned this Oct 10, 2025
@@ -0,0 +1,325 @@
{
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #2.    # !pip install imdclient>=0.2.2

Don't we want people to install with Conda for consistency?

Also, will people be running this notebook, or reading it as a guide? I think this cell can potentially be removed, seems like a bit of overkill


Reply via ReviewNB

Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need package installation here. imdclient will always be installed when MDA is installed. imdclient will need to be installed separately. However, I'd mention this fact and then link to installation instructions instead of repeating instructions that may change over time.

In general, always link to authoritative instructions; you're the expert on the matter so you should know what the authoritative sources are.

@@ -0,0 +1,325 @@
{
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is IMD-coords essential for GROMACS? Will the stream fail out without it? Or do you just mean its essential for this example?

Maybe it's worth specifying exactly what's necessary for this example: you need to have coordinates turned on regardless of simulation engine, you need to have freq set to 1, and you need to turn off coordinate unwrapping (so that there are consistent configs across engines)

Also, should this link the docs for each of the simulation engines?


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to

@@ -0,0 +1,325 @@
{
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #42.    # simple_imd_analysis()

Why is this commented out? Will github run it automatically as a part of docstring tests if it's uncommented?


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notebook needs to be primarily a readable document, don't expect people to edit it. Thus, it should show _working code_, so avoid "un-commenting something so that it works".

If there are issues with it being run, comment on the PR and then let's look for a solution.

@@ -0,0 +1,325 @@
{
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #41.                potential_energy = -1000 + np.random.normal(0, 50)

Not all simulation engines implement energy packet, so maybe remove this?


Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true but for the ones that do, we should show how one can get the information.

@@ -0,0 +1,325 @@
{
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #51.                    ax1.clear()

Just double checking, did you test this method of live updating plot locally? Looks a bit different than what we did in workshop

https://github.com/MDAnalysis/imd-workshop-2024/blob/main/activity/graph_utils.py


Reply via ReviewNB


.. code-block:: bash

pip install imdclient

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we encourage conda/mamba installation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the required change!

print(f"WARNING: Large displacement detected at {ts.time} ps: {max_displacement:.2f} Å")

# Monitor energies if available
if 'potential' in ts.data:
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these examples I might not even build in safe guards but just state at the top that you're assuming that energy is transmitted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


* ``dt``: Time step size in picoseconds
* ``step``: Current simulation step number
* Energy terms: ``potential``, ``total``, etc. (engine-dependent)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these keys are wrong, see:

https://github.com/Becksteinlab/imdclient/blob/07069e1ff08d1488936c61e52f922116ecddf210/imdclient/IMDClient.py#L956

I think we actually merged them into MDA develop docstring for IMDReader incorrectly:

https://imdclient.readthedocs.io/en/latest/protocol_v3.html#energies

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are these engine-dependent? It's engine depended whether the energy packet is implemented, but the exact energy keys should be consistent whenever the packet is implemented, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljwoods2 can you raise an issue, please?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljwoods2 - yes, the keys should be the same for all engines, only implementation availability is engine dependent - I chnage the langauage around it

Thanks for pointing out the erros in the API documentation. I'll make a PR to correct it!

if key not in ['dt', 'step']:
print(f" {key}: {value}")

Multiple Client Connections
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a ton of edge cases when using multiple clients that we don't test, like when one client tries to change blocking behavior, so I think we should just say it isn't recommended

Discussion here

Becksteinlab/imdclient#114

Maybe should update MDA main codebase docs to say this as well:

https://github.com/amruthesht/mdanalysis/blob/ea85bb510f684ef1f6048ad0a50219665298cbb3/package/MDAnalysis/coordinates/IMD.py#L108

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave out the whole section; perhaps mention somewhere that it may be possible but results are not well defined and link to imdclient docs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwoods2 - I actually tested some edge cases out - I'll add it to the relevant PR discussion.

@orbeckst - Done!


Most MDAnalysis analysis classes work with streaming data, but some limitations apply:

**Compatible Analysis**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should start by outlining what kinds of analyses will and won't work before jumping into distance and contact analysis

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - could you check if this is accurate, thanks!

analysis_data.append(calculate_something(ts))

# This will NOT work - random access
ts = u.trajectory[10] # TypeError
Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading docstring of __getitem__ of StreamReaderBase, I think these should both be ValueErrors

def __getitem__(self, frame):
        """Return an iterator for slicing a streaming trajectory.

        Parameters
        ----------
        frame : slice
            Slice object. Only the step parameter is meaningful for streams.

        Returns
        -------
        FrameIteratorAll or StreamFrameIteratorSliced
            Iterator for the requested slice.

        Raises
        ------
        TypeError
            If frame is not a slice object.
        ValueError
            If slice contains start or stop values.

If it does throw a TypeError, something is wrong...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks for pointing it out!

# This will NOT work - restarting iteration
for ts in u.trajectory:
break
for ts in u.trajectory: # Won't start from beginning

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggest it will start from somewhere else other than the beginning, won't it just fail?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it just start from the next available frame in the buffer?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it shouldn't if I'm understanding the code right (just trying to run a for loop two times), it should raise an error:

https://github.com/MDAnalysis/mdanalysis/blob/03eef459ddf83e73a776c0e6ddefde4a04ce62c9/testsuite/MDAnalysisTests/coordinates/test_imd.py#L457

print(f"Time: {ts.time:.2f} ps, Step: {ts.data.get('step', 'N/A')}")

# Your analysis code here
selected_atoms = u.select_atoms("protein and name CA")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify -- is there something about IMD that mandates repeating the selection of the AtomGroup in the loop, or can this be hoisted up for efficiency as usual?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing unique about IMD here, can be changed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amruthesht move selection out of the loop to model best practices (it's bad for performance!)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylerjereddy - nothing specific about IMD requires it. I'll modify the example so it doesn't give the wrong impression

print(f"Protein COM: {center_of_mass}")

# Optional: break on some condition
if ts.time > 1000: # Stop after 1000 ps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify: this only stops the analysis, not the running simulation/engine the analysis is connected to?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this break is the end of the script, it will trigger the IMDClient to cleanup via atext

https://github.com/Becksteinlab/imdclient/blob/07069e1ff08d1488936c61e52f922116ecddf210/imdclient/IMDClient.py#L146

That cleanup might put the simulation engine in a waiting state for another client connection, but won't kill it.

Whether it makes the simulation engine wait or not depends on 1. how the simulation engine was configured and 2. whether the IMDClient overrode the simulation engine's waiting behavior

continue_after_disconnect kwarg is the IMDClient switch for changing waiting behavior

https://imdclient.readthedocs.io/en/latest/api.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the need to look out for this/think about this might be sensible to mention somewhere inline here then, if only to refer to more detailed docs for someone who may be copy/pasting the sample code as a template.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the optional part from this example so that it can stand as a simple example on its own.

Then create another example where you discuss the issue with how breaking the loop may affect the running simulation. This is important but nuanced. If something is complicated, break it into smaller steps, that's basic pedagogy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still recommend breaking up this example

I'd remove the optional part from this example so that it can stand as a simple example on its own.

Then create another example where you discuss the issue with how breaking the loop may affect the running simulation. This is important but nuanced. If something is complicated, break it into smaller steps, that's basic pedagogy.


# Contact analysis
selection1 = u.select_atoms("resid 1-10")
selection2 = u.select_atoms("resid 50-60")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hoist selections if IMD allows it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


* **No timeseries methods**: Cannot use ``trajectory.timeseries()``
* **No bulk operations**: Cannot extract all data at once
* **Limited multiprocessing**: Cannot split across processes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a concrete example of something you can do with multiprocessing with "vanilla" MDAnalysis that doesn't work with IMD? I would have figured that if you can get data into a NumPy array you're probably "ok" to split off additional processes if you want to.

Copy link

@ljwoods2 ljwoods2 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By vanilla MDA, does that exclude analysis classes with dask/multiprocessing backend?

You could copy the entire traj into np array as it goes, then create universe with traj from array, but that would defeat the purpose I think

And you can use multiprocessing/parallelism on a single frame / sliding window of frames, I know @HeydenLabASU has tried some windowing and then using parallel algorithms to process an i.e. 200 frame window

I guess maybe this should be changed to say that it only works with the serial analysis backend

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would defeat the purpose I think

I guess you could "buffer"/store a few frames in an array that way without doing IO, and possibly only for a select subset of particles, which does seem within the remit/spirit of what IMD enables, or possibly even one of the more interesting use cases. I think that's what you're saying Matthias is doing.

I might guess that accessing multiple frames in parallel is a problem for the same reason that random access is not available, and that some analyses that require multiple passes and divide work over random-access frame access are also banned, so yeah just trying to think of cases that block multiprocessing usage that aren't already covered by the elementary limitations mentioned. Maybe the multiple clients in different processes thing, but I think you also already ruled them out in comments above, whether in different processes or not.

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include links to the authoritative docs — imdclient and IMDReader in the main docs.

The imdclient package should be installed as a dependency of MDAnalysis (in 2.10.0...IIRC(


Reply via ReviewNB

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all codes:

1) Do we want to send IMD data every integrator step in the example? It's ok to do so but write some text above that says explicitly that we are doing it in the example but in production simulations this may be every 5 or every 10 steps.

For GROMACS:

2) Let's model best practice and encourage everyone to send time and box information so that this is always available in MDAnalysis:

IMD-time         = Yes         ; Send time information
...
IMD-box          = Yes         ; Send box dimensions (recommended)

Reply via ReviewNB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section appears to be a duplicate of what's in the imd.rst. Can you just omit all of it and just link?

It's basically impossible to keep documentation in sync so the best you can do is to only have it in a single place and link — DRY, don't repeat yourself.

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain what analysis you're performing. Link to https://docs.mdanalysis.org/stable/documentation_pages/core/groups.html#MDAnalysis.core.groups.AtomGroup.radius_of_gyration (or somewhere in the User Guide) for background.

Also explain that **as an example** you only process the first 10 frames but normally you'd read all frames.

You should also discuss

  • skipping (trajectory[::5])
  • limitations (no start/stop)

Reply via ReviewNB

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the structure of the code in that the contents of the try block are the whole code. If you want to protect code then make a function out of the inner block. However, I would just not use the try/except and just state that the example requires a running simulation.

(Additionally, I don't like catching all exceptions and then just printing an error — this is bad practice because errors can slip through.)


Reply via ReviewNB

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #21.            frame_count = 0

In the text, explain that one of the limitations is that one cannot use trajectory[:10] to make clear why you're keeping track of frames. (You can link to StreamReaderBase for more details on limitations).

By the way, couldn't you use ts.frame? That would at least not be quite as clunky.


Reply via ReviewNB

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #15.        try:

Same comment as above: kind of ugly to have so much code inside try/except.

I'd rewrite the code block inside the try block as a function and then keep the existing try/except structure to demonstrate the overall control structure.


Reply via ReviewNB

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #41.                potential_energy = -1000 + np.random.normal(0, 50)

Why do we fake it? Instead we should get it from the energies packet. You just need to say above that one needs to enable sending of energies and then pull the correct data out of the ts.data. This would be a good example for showing how to do it, don't squander the opportunity.


Reply via ReviewNB

@@ -0,0 +1,325 @@
{
Copy link
Member

@orbeckst orbeckst Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #96.    # advanced_live_streaming()

No commented out code, see above.


Reply via ReviewNB

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start for the User Guide. I added a bunch of comments in addition to Tyler's and Lawson's ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment: link to the primary sources (imdclient docs, IMDReader docs)


.. include:: classes/IMD.txt

Real-time streaming of simulation data between molecular dynamics engines and receiving clients can be achieved using IMD protocols like IMDv2 and IMDv3. The :class:`~MDAnalysis.coordinates.IMD.IMDReader` implements the IMDv3 protocol, enabling live streaming of ongoing simulation data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to explain what the general purpose is (re-summarize paper, IMDReader, imdclient docs). Put yourself in the position of someone whose first contact with the idea is through the User Guide. Answer the questions

  • What is it?
  • Why is it useful?
  • Where can I learn more?

Also explain abbreviation "IMD".

Recyle what you've already have written, e.g. from the "What is streaming" section, which is already great in answering the first question. You just need to taylor the initial 2-3 sentences so that they make sense on their own.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

What is Streaming?
==================

Streaming involves processing data in real-time as it is generated, rather than storing it for later analysis. In molecular dynamics, this means sending simulation data to a client on-the-fly while the simulation is running, without writing large trajectory files to disk.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good!


MDAnalysis's :class:`~MDAnalysis.coordinates.IMD.IMDReader` uses the `imdclient <https://imdclient.readthedocs.io/>`_ package and provides a familiar interface for reading streaming data, similar to other trajectory readers in MDAnalysis.

When to Use Streaming?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice — answers my questions from the first paragraph: so summarize in the first paragraph at a high level what you're writing here.


Streaming analysis has fundamental constraints due to its real-time nature:

**Data Access Limitations**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use proper section markup for subheadings

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

* **No trajectory length**: Total frame count unknown until simulation ends
* **No independent copies**: Cannot create multiple reader instances for the same stream

**Analysis Constraints**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use proper section markup for subheadings

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

* **Limited multiprocessing**: Cannot split across processes
* **Single client**: Only one reader per IMD stream (engine-dependent)

**Practical Considerations**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use proper section markup for subheadings

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Comment on lines 307 to 312
# This WILL work - forward iteration
for ts in u.trajectory:
analysis_data.append(calculate_something(ts))

# This will NOT work - random access
ts = u.trajectory[10] # TypeError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split into separate code blocks with preceding text that states what is being attempted and what will happen; the comments "will work/won't work" can also stay to make it clear.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

orbeckst and others added 2 commits October 15, 2025 17:23
Co-authored-by: ljwoods2 <145226270+ljwoods2@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
-Changes to the docs based on comments
Co-authored-by: ljwoods2 <145226270+ljwoods2@users.noreply.github.com>
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reST document looks pretty good to me (I think one major comment is unresolved). The notebook still has many unresolved comments as far as I can tell.

if ts.time > 1000: # Stop after 1000 ps
break

The ``buffer_size`` parameter (10 MB = 10*1024*1024 bytes in this example) controls the buffer used by `imdclient <https://imdclient.readthedocs.io/>`_ to temporarily store data received from the socket. This buffer accounts for speed differences between the producer (simulation engine) and receiver (analysis code), preventing data loss when analysis is slower than data transmission. A larger buffer is more suitable for systems with many atoms or high transmission frequencies. For more details on buffer management and optimization, see the `imdclient documentation <https://imdclient.readthedocs.io/>`_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I would try to replace in-line hyperlinks with named links just to keep things tidy, especially if you're using the same URL multiple times. (Optional)

print(f"Protein COM: {center_of_mass}")

# Optional: break on some condition
if ts.time > 1000: # Stop after 1000 ps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still recommend breaking up this example

I'd remove the optional part from this example so that it can stand as a simple example on its own.

Then create another example where you discuss the issue with how breaking the loop may affect the running simulation. This is important but nuanced. If something is complicated, break it into smaller steps, that's basic pedagogy.

@orbeckst
Copy link
Member

orbeckst commented Oct 30, 2025

RTD installation fails because 3.10 not supported by MDA anymore. (Raised #431 )

@orbeckst
Copy link
Member

@amruthesht a simple thing to address are the issues from pre-commit https://github.com/MDAnalysis/UserGuide/actions/runs/18957769421/job/54138651607?pr=430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add UserGuide documentation for IMDReader - reading streaming data with IMDv3 protocol

4 participants