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.

Sign up for GitHub

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

Jump to bottom

'MDAnalysis.analysis.nucleicacids' parallelization #4727

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

'MDAnalysis.analysis.nucleicacids' parallelization #4727

wants to merge 11 commits into from
+40 −14

Conversation

talagayev
Copy link
Member

@talagayev talagayev commented Oct 7, 2024 •
edited by github-actions bot
Loading

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/

@talagayev
Update nucleicacids.py
b00d502 
added backend and aggregators
@talagayev
Update conftest.py
dc03a02 
added NucPairDist to the conftest.py
@talagayev
Update test_nucleicacids.py
39612c8 
added client_NucPairDist to the tests
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2024 •
edited
Loading

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 •
edited
Loading

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-10-14 18:28:04 UTC

@codecov Codecov
Copy link

codecov bot commented Oct 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.58%. Comparing base (740e74e) to head (c722ac4).
Report is 1 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4727      +/-   ##
===========================================
+ Coverage    93.55%   93.58%   +0.02%     
===========================================
  Files          173      185      +12     
  Lines        21463    22534    +1071     
  Branches      3987     2993     -994     
===========================================
+ Hits         20080    21088    +1008     
- Misses         929     1002      +73     
+ Partials       454      444      -10

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

yuxuanzhuang
yuxuanzhuang requested changes Oct 14, 2024
View reviewed changes
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 :)

@talagayev
Update nucleicacids.py
b2d81b0 
fixing PEP
@talagayev
Update nucleicacids.py
e30716b
@talagayev
Update nucleicacids.py
883eb1e 
added versionchanged for addition of parallelization
@talagayev
Update nucleicacids.py
701ed01
@talagayev
Update conftest.py
82a89f9
@talagayev
Update CHANGELOG
886832e 
Added Parallelization of nucleicacids.py and fixed lettering
@talagayev
Update nucleicacids.py
c722ac4 
Addition of mention of modification to self.results.distances
@talagayev talagayev marked this pull request as ready for review October 14, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuxuanzhuang yuxuanzhuang yuxuanzhuang requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MDAnalysis.analysis.nucleicacids: Implement parallelization or mark as unparallelizable
3 participants
@talagayev @pep8speaks @yuxuanzhuang