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

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

Line 310:80: E501 line too long (86 > 79 characters)
Line 327:80: E501 line too long (87 > 79 characters)
Line 333:80: E501 line too long (97 > 79 characters)
Line 357:80: E501 line too long (85 > 79 characters)
Line 377:80: E501 line too long (91 > 79 characters)

Comment last updated at 2025-01-11 21:40:18 UTC

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.73%. Comparing base (b62e2f7) to head (5009b02).
⚠️ Report is 2 commits behind head on develop.

@@           Coverage Diff            @@
##           develop    #4738   +/-   ##
========================================
  Coverage    92.72%   92.73%           
========================================
  Files          180      180           
  Lines        22472    22491   +19     
  Branches      3188     3191    +3     
========================================
+ Hits         20837    20856   +19     
  Misses        1177     1177           
  Partials       458      458           

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

@marinegor I would ping you in this PR as well.

Here basically I tried different ways to see if it is possible to parallelize the AverageStructure and AlignTraj classes.
I was able to implement the parallelization for AverageStructure with the first attempt not working due to being able to read a universe so that error appeared

AttributeError: 'numpy.ndarray' object has no attribute 'load_new'

I added _first to make the class parallelizable. This works well with the test, expect the case, when in_memory=True, then the process takes very long, which I assume is connected to memory issues, so in the current case I revert back to serial for cases when in_memory=True.

As for AlignTraj due to it transforming coordinates and writing out structures it would be necessary to rewrite more parts of the code to make it parallelizable, so I didn't find an easy solution for that, which wouldn't require bigger modifications of the class. So there the question would be if we keep it as non parallelizable or should I try to modify the code to make it parallelizable, which would require bigger modifications?

@talagayev

expect the case, when in_memory=True, then the process takes very long, which I assume is connected to memory issues

I think parallelization should not actually work with in_memory cases (@yuxuanzhuang please correct me if I'm wrong, afaik you've been working on this). Hence I'd explicitly raise an exception if uses asks for parallel execution and provides in_memory as well.

So there the question would be if we keep it as non parallelizable or should I try to modify the code to make it parallelizable, which would require bigger modifications?

If you're running out of ideas, I'd suggest making this PR for AverageStructure only, and create appropriate issue for AlignTraj, describing your attempts so far.

Also, I imagine there are problems with serialization of self._writer, no? Perhaps we can chat on discord about it (I'm @marinegor there)?

@talagayev

expect the case, when in_memory=True, then the process takes very long, which I assume is connected to memory issues

I think parallelization should not actually work with in_memory cases (@yuxuanzhuang please correct me if I'm wrong, afaik you've been working on this). Hence I'd explicitly raise an exception if uses asks for parallel execution and provides in_memory as well.

So there the question would be if we keep it as non parallelizable or should I try to modify the code to make it parallelizable, which would require bigger modifications?

If you're running out of ideas, I'd suggest making this PR for AverageStructure only, and create appropriate issue for AlignTraj, describing your attempts so far.

Also, I imagine there are problems with serialization of self._writer, no? Perhaps we can chat on discord about it (I'm @marinegor there)?

Yes makes sense. I think the current two ones that use in_memory and are analysis related are AverageStructure and AlignTraj.

Yes that would be good, I can then rename the PR to cover only AverageStructure for now, add the missing parts for the PR (Documentation + Changelog), create an Issue and write you on Discord, so that we can brainstorm how to adjust the code to make it parallelizable. Yes self._writer is one of the difficulties. I guess for the aligntraj you can adjust the code to give it the reference, but yes the writing during parallelization is the tricky part, maybe with tmp information that is then merged or maybe just doing the calculations and the writing is then in conclude, basically keeping that part serial and only making the calculations parallel.

@talagayev ok, will be waiting for your message.
I also assigned myself a reviewer here, so just re-request review when you think you're done!

@talagayev ok, will be waiting for your message. I also assigned myself a reviewer here, so just re-request review when you think you're done!

Added the Documentation, CHANGELOG and adjust to raise and ValueError. The PR would be ready to be re-reviewed :)