Parallelizes MDAnalysis.analysis.InterRDF and MDAnalysis.analysis.InterRDF_s

[tanishy7777]

Fixes #4675

Changes made in this Pull Request:

• Parallized both rdf.InterRDF and rdf.InterRDF_s

TLDR: of the comments below: Initially I thought rdf isnt parallizable but turns out both classes in rdf can be parallelized.

PR Checklist

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

Developers certificate of origin

• I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

---

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

[pep8speaks]

Hello @tanishy7777! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/analysis/rdf.py:

Line 592:1: W293 blank line contains whitespace

• In the file testsuite/MDAnalysisTests/analysis/test_rdf.py:

Line 160:1: E302 expected 2 blank lines, found 1

• In the file testsuite/MDAnalysisTests/analysis/test_rdf_s.py:

Line 179:1: E302 expected 2 blank lines, found 1

Comment last updated at 2025-01-20 20:35:32 UTC

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.86%. Comparing base (79dead3) to head (2c3c785).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4884   +/-   ##
========================================
  Coverage    93.86%   93.86%           
========================================
  Files          179      179           
  Lines        22217    22242   +25     
  Branches      3157     3159    +2     
========================================
+ Hits         20853    20878   +25     
  Misses         902      902           
  Partials       462      462           

☔ 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.

[marinegor]

@tanishy7777 thanks for the prompt PR! I have some comments though:

self.volume_cum is cummulated across the frames so we cant parallize simply using the split-apply-combine technique.

Can we just sum it up separately though? I mean, make it a part of self.results for each worker's trajectory, and in _conclude write sum of it to self.volume_cum.

[tanishy7777]

Can we just sum it up separately though? I mean, make it a part of self.results for each worker's trajectory, and in _conclude write sum of it to self.volume_cum.

Got it! I have made analysis.rdf.InterRDF parallizable with this approach but analysis.rdf.InterRDF_s needs a bit more work.

[tanishy7777]

when trying to make analysis.rdf.InterRDF_s parallizable I am running into an error with aggregating results.count.

FAILED testsuite\MDAnalysisTests\analysis\test_rdf_s.py::test_nbins[client_InterRDF_s1] - ValueError: setting an array element with a 
sequence. The requested array has an inhomogeneous shape after 1 dimensions. The detected shape was (2,) + inhomogeneous p...

I tried to find out why this is happening by looking at the result itself which is being aggregated along 'count'

so the arrs that is passed to the ResultsGroup.ndarray_vstack function as shown below can be broken into arrs = [arr, arr1] (refere imgs)

so I tried finding the dimensions of the arrays manually because it wasnt converting to a numpy array.

so its not able to convert to a numpy array, because of inconsistent dimensions. I am not sure how to resolve this

here arr is basically made up of of 2 arrays of (1,2,412) and (2,2,412)
and arr1 is also made up of of 2 arrays of (1,2,412) and (2,2,412)

[tanishy7777]

based on the above comment #4884 (comment)

I think we can mark analysis.rdf.InterRDF_s as non parallizable and mark analysis.rdf.InterRDF as parallizable

because its not possible to convert array of inhomogenous dimensions to a numpy array and since rdf.InterRDF_s needs numpy arrays to be passed. so even if we concatenate the arrays as normal lists performing even basic operations would be hard.

Because after the _get_aggregator method when _conclude is run operations like

self.results.count[i] / norm

wont be possible as division is not supported between list and int

[marinegor]

@tanishy7777 the class should be marked as non-parallelizable only if the algorithm to run it is not actually parallelizable, which I'm not yet convinced is the case for all the mentioned classes.

But I think you're on the right path here, you just need to implement a custom aggregation function, instead of those implemented among ResultsGroup staticmethods -- they obviously don't cover all the possible cases for aggregation, just the basic ones for user's convenience.

Can you describe what kind of arrays you're trying to aggregate and can not find an appropriate function for? I didn't quite get it from your screenshots.

[tanishy7777]

But I think you're on the right path here, you just need to implement a custom aggregation function, instead of those implemented among ResultsGroup staticmethods -- they obviously don't cover all the possible cases for aggregation, just the basic ones for user's convenience.

Can you describe what kind of arrays you're trying to aggregate and can not find an appropriate function for? I didn't quite get it from your screenshots.

So, the array is something like this

The [412] denotes an array with 412 elements.

That is why it cant be processed by numpy directly. But I think if I modify it, using a custom comparator like you mentioned I can sum the entries(by adding the 3 blue arrays of shape 2x412 as in the picture) and convert it to a 1x2x412 array I think? I am not sure about the final dimension it needs to be converted to.

@marinegor

[marinegor]

I am not sure about the final dimension it needs to be converted to

it should be the same as if you'd run it without parallelization. And I assume you want to stack/sum/whatever along the dimension that corresponds to the timestep -- you can probably guess which one it is if you run it on some example with known number of frames. Example trajectories you can find in MDAnalysisTests.

[tanishy7777]

it should be the same as if you'd run it without parallelization. And I assume you want to stack/sum/whatever along the dimension that corresponds to the timestep -- you can probably guess which one it is if you run it on some example with known number of frames. Example trajectories you can find in MDAnalysisTests.

Got it. Will work on that!

[tanishy7777]

it should be the same as if you'd run it without parallelization. And I assume you want to stack/sum/whatever along the dimension that corresponds to the timestep -- you can probably guess which one it is if you run it on some example with known number of frames. Example trajectories you can find in MDAnalysisTests

I tried making the custom_aggregator but some tests are still failing

These 2 lines specifically are causing the errors in all the 12 tests
assert_allclose(max(rdf.results.rdf[0][0][0]), value)
assert rdf.results.edges[0] == rmin

I am not sure how to resolve this, I will dig through the docs a bit more. Will update if I can figure it out.

[tanishy7777]

used a custom aggregator for results.counts and converted the aggregation method for
results.edges and results.bins to ResultsGroup.ndarray_mean

TLDR: Parallized both InterRDF and InterRDF_s classes in rdf.py

Thanks @marinegor for your help!

[tanishy7777]

@orbeckst @RMeli @marinegor I think this is ready to be merged, can you please review it?

[marinegor]

hi @tanishy7777, sorry for the long review (again).

good work, thanks for your contribution! I've added my comments, main action points below:

• move your custom aggregation function from the class to a standalone function, name appropriately and test
• revert changes to test_xds.py and core/selection.py (I'm guessing they were introduced by black or smth)
• make sure you don't need to track self.volume_cum, since you're tracking self.results.volume_cum and assigning self.volume_cum in _conclude. I made suggestions regarding that but might have missed something; please make sure until _conclude only self.results.volume_cum is used.

[tanishy7777]

hi @tanishy7777, sorry for the long review (again).

good work, thanks for your contribution! I've added my comments, main action points below:

• move your custom aggregation function from the class to a standalone function, name appropriately and test
• revert changes to test_xds.py and core/selection.py (I'm guessing they were introduced by black or smth)
• make sure you don't need to track self.volume_cum, since you're tracking self.results.volume_cum and assigning self.volume_cum in _conclude. I made suggestions regarding that but might have missed something; please make sure until _conclude only self.results.volume_cum is used.

Sorry for the late reply. I will get to work on these changes! I had semester examinations so was quite busy the last 2 weeks.

[tanishy7777]

@marinegor Sorry for the delayed action on this PR again, college has been quite hectic.

I have added the changes that you had requested. For the formatting issues caused by black, I merged develop into my branch so I think it should be resolved.

Please let me know if any further changes are needed. Thanks for reviewing my changes and for your guidance!

[orbeckst]

@tanishy7777 thanks for all your contributions. As you know, reviewer time is pretty precious. I asked @marinegor to look at PR #4896 already so please be understanding if this one may take longer.

[tanishy7777]

@tanishy7777 thanks for all your contributions. As you know, reviewer time is pretty precious. I asked @marinegor to look at PR #4896 already so please be understanding if this one may take longer.

Thanks for you reply. Completely understandable, no worries at all.

[orbeckst]

@tanishy7777 just as a heads-up: Egor told us he has very little availability, so we'll have to find other reviewers to continue here.

As the first step where you can help is to just resolve the conflict to get the PR back into mergeable state.

[orbeckst]

@p-j-smith would you be in a position to review and look after this parallelization PR? Egor has already done a lot of deep reviewing here but as he can't do it at the moment and we'd like to move this improvement along, it would be great to have your 👀 on it.

I am tentatively assigning you but if you can't do it, please un-assign yourself and let me know with a comment + ping and then I'll be looking for someone else. Thanks!!

[p-j-smith]

Very nice work @tanishy7777 - just a couple of small changes to simplify the tests. And please merge in the latest changes from develop

[p-j-smith]

is this needed?

[p-j-smith]

you don't need to parametrise this test, this would be equivalent:

def test_class_is_parallelizable():
    assert mda.analysis.rdf.InterRDF._analysis_algorithm_is_parallelizable

[p-j-smith]

same here, this doesn't need to be parametrised. Also, I'm not sure we need this test - if we want to update the supported backends at some point, we will also need to update the test. Or is that the point of the test?

[p-j-smith]

these tests don't need to be parametrised