Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'MDAnalysis.analysis.nucleicacids' parallelization #4727

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

talagayev
Copy link
Member

@talagayev talagayev commented Oct 7, 2024

Fixes #4670 attempt

Changes made in this Pull Request:

  • added backends and aggregators to NucPairDist in analysis.nucleicacids
  • added the client_NucPairDist in conftest.py
  • added client_NucPairDist in run() in test_nucleicacids.py

Here the Error:

AttributeError: 'WatsonCrickDist' object has no attribute '_res_array' appears for all of the tests where the client_NucPairDist is used, same goes not only for WatsonCrickDist, but also for MinorPairDist and MajorPairDist.

I am a little bit stuck, since I am not sure if I need to modify the analysis.nucleicacids to fix this error, thus I made this PR as a draft to get more input and ideas how to fix this issue :)

PR Checklist

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

Developers certificate of origin


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

added backend and aggregators
added NucPairDist to the conftest.py
added client_NucPairDist to the tests
Copy link

github-actions bot commented Oct 7, 2024

Linter Bot Results:

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

There are currently no issues detected! 🎉

@yuxuanzhuang yuxuanzhuang self-requested a review October 10, 2024 00:08
@pep8speaks
Copy link

pep8speaks commented Oct 10, 2024

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

Line 178:1: W293 blank line contains whitespace

Comment last updated at 2024-10-23 00:47:12 UTC

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.62%. Comparing base (740e74e) to head (c68cb84).
Report is 7 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4727      +/-   ##
===========================================
+ Coverage    93.55%   93.62%   +0.07%     
===========================================
  Files          173      187      +14     
  Lines        21463    22634    +1171     
  Branches      3987     3023     -964     
===========================================
+ Hits         20080    21192    +1112     
- Misses         929      998      +69     
+ Partials       454      444      -10     

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

Copy link
Contributor

@yuxuanzhuang yuxuanzhuang left a comment

Choose a reason for hiding this comment

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

Looking good! Could you also help fix the pep8?

package/MDAnalysis/analysis/nucleicacids.py Show resolved Hide resolved
package/MDAnalysis/analysis/nucleicacids.py Show resolved Hide resolved
@talagayev
Copy link
Member Author

Looking good! Could you also help fix the pep8?

Yes, I have now access again to a PC, although with slow internet, so I will adjust it for pep8 and also add the additional stuff that is missing (changelog etc.).

I would also create an additional PR draft for analysis.align, since there I am not sure if it can be parallelized :)

added versionchanged for addition of parallelization
Added Parallelization of nucleicacids.py and fixed lettering
Addition of mention of modification to self.results.distances
@talagayev talagayev marked this pull request as ready for review October 14, 2024 19:09
Copy link
Contributor

@yuxuanzhuang yuxuanzhuang left a comment

Choose a reason for hiding this comment

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

lgtm!

package/MDAnalysis/analysis/nucleicacids.py Show resolved Hide resolved
removed _res_dict
moved back _res_dict
removed _res_sel
remove unecessary ,
@orbeckst
Copy link
Member

@yuxuanzhuang thanks for the review. Could you please shepherd the PR to completion?

(I think we're in feature-freeze for 2.8.0 #4733 but check the issue for updates.)

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

Successfully merging this pull request may close these issues.

MDAnalysis.analysis.nucleicacids: Implement parallelization or mark as unparallelizable
5 participants