'MDAnalysis.analysis.density' parallelization

[talagayev]

Fixes #4677

Changes made in this Pull Request:

• added backends and aggregators to DensityAnalysis in analysis.density
• added client_DensityAnalysis in conftest.py
• added client_DensityAnalysis to the tests in test_density.py

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--4729.org.readthedocs.build/en/4729/

[pep8speaks]

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

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

Line 426:66: W291 trailing whitespace
Line 427:68: W291 trailing whitespace
Line 428:36: W291 trailing whitespace

Comment last updated at 2024-12-18 19:05:59 UTC

[github-actions]

Linter Bot Results:

Hi @talagayev! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/11265261686/job/31326801780

---

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.63%. Comparing base (a3672f2) to head (506597d).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4729      +/-   ##
===========================================
- Coverage    93.65%   93.63%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21774    22845    +1071     
  Branches      3064     3064              
===========================================
+ Hits         20393    21391     +998     
- Misses         929     1002      +73     
  Partials       452      452              

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

[yuxuanzhuang]

Three aspects in my fix.

• Attributes (private or not) that will be used later in _conclude should be defined in __init__, not in _prepare. @marinegor, I think it would be helpful to explicitly mention this in the documentation. Additionally, while the illustrative figure (https://docs.mdanalysis.org/dev/documentation_pages/analysis/parallelization.html#split-apply-combine) suggests that _prepare occurs before splitting, this is not the case.

• The _edges and similar attributes must be defined in __init__ by nature because they need to be consistent across all workers and are determined based on the current positions of the atomgroup (https://github.com/MDAnalysis/mdanalysis/pull/4729/files#diff-28b7c110c6231f9f2ea864bbdecb9cb485edbed494d7e266d37896bde7b50b0dR421) .

• The results being aggregated are actually _grid, not Density—Density serves more as a postprocessing step that summarizes the information.

[orbeckst]

@yuxuanzhuang could you please raise issues for the updates to the parallelization documentation (#4729 (comment)) — I think that's very important to make clear, given that we are now repeatedly running in these issues. Thank you!

[marinegor]

@yuxuanzhuang thanks, the illustration is indeed wrong, as well as the accompanying text. I fixed them in #4760

[orbeckst]

As we've been figuring out: data structures need to be laid out in __init__, thus the size of _grid needs to be set up before _prepare (which is executed in each worker process).

[orbeckst]

The overall grid shape needs to be determined in __init__, then each worker can have its own.

[talagayev]

Ah okay, yes that makes sense.

I think there was also an additional case, where I encountered this problem with the parallelization;
I will try to apply the same fix there to see if it works.

Other than that I added the Changelog and the docs, so should be ready to go :)

[orbeckst]

This looks fine.

I am adding a single comment, otherwise my comments on the code are just observations.

[orbeckst]

This looks like the right change.

I am noticing that the necessary changes for parallelization really shift a lot of what _prepare() was doing back into __init__. I guess that's ok and it's better than introducing yet another method like _prepare_init() ... We probably need to review the docs for writing analysis code with AnalysisBase.

[orbeckst]

Add a comment so that future readers know why we are doing everything here
and not in _prepare().

Suggested change

	def _prepare(self):
	# The grid with its dimensions has to be set up in __init__
	# so that parallel analysis works correctly: each process
	# needs to have a `results._grid` of the same size and the
	# same `_bins`.

[orbeckst]

manually added to the file, suggestions don't work on deleted lines

[orbeckst]

I am not 100% sure if we need to test warnings for all parallel modalities, too, but it's the safe approach and you've already converted all tests so let's leave it in. Just something to think about in the future if our test run times are too long (again...).