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

Implementation of parallelization to MDAnalysis.analysis.dssp #4721

Merged
merged 12 commits into from
Oct 7, 2024

Conversation

talagayev
Copy link
Member

@talagayev talagayev commented Sep 27, 2024

Fixes #4674

Changes made in this Pull Request:

  • Parallelization of the backend support to the class DSSP in dssp.py
  • Addition of versionchanged to mention the addition of the parallelization
  • Addition of parallelization tests in test_dssp.py and fixtures in conftest.py
  • Updated Changelog

PR Checklist

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

Developers certificate of origin


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

Added backends and aggregators
Added client_DSSP
Added the client_DSSP to the tests
Added DSSP parallelization to the changelog
Added the versionchanged to the class
@pep8speaks
Copy link

pep8speaks commented Sep 27, 2024

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

Line 138:1: W293 blank line contains whitespace

Comment last updated at 2024-10-07 17:46:24 UTC

Copy link

github-actions bot commented Sep 27, 2024

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 ✅ Passed
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/11221046796/job/31190523020


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

adjusting dssp.py for PEP
Adjusting for PEP
black adjustment
removed whitespaces
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.53%. Comparing base (474be5b) to head (3c6cbec).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4721      +/-   ##
===========================================
- Coverage    93.55%   93.53%   -0.02%     
===========================================
  Files          173      185      +12     
  Lines        21457    22529    +1072     
  Branches      3986     3987       +1     
===========================================
+ Hits         20074    21073     +999     
- Misses         929     1002      +73     
  Partials       454      454              

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

lgtm!

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

I would have then also have a quick question:
for example when I try to add the parallelization to desnity.py for the DensityAnalysisI get this during the test_density.py pytests as a Failure message for test_userdefn_boxshape:

AttributeError: 'DensityAnalysis' object has no attribute '_grid'

I am not sure how to fix such cases, should I just create a PR so that the reviewers can take a look at it and suggest a fix?

@RMeli
Copy link
Member

RMeli commented Oct 1, 2024

@talagayev it would help to see the full backtrace for the error, otherwise it's a bit hard to help.

should I just create a PR so that the reviewers can take a look at it and suggest a fix?

This might be useful if you are planning to work on it regardless. You can open a PR as "draft", to make it explicitly it is work in progress but want early feedback/need some help/want to test in CI.

@talagayev
Copy link
Member Author

@talagayev it would help to see the full backtrace for the error, otherwise it's a bit hard to help.

should I just create a PR so that the reviewers can take a look at it and suggest a fix?

This might be useful if you are planning to work on it regardless. You can open a PR as "draft", to make it explicitly it is work in progress but want early feedback/need some help/want to test in CI.

hey @RMeli, perfect that is what i was looking for, thanks :) I will then create the PR as a draft to get some feedback, since there are also some other cases like MDAnalysis.analysis.align where I tried to parallelize them and think that they are not possible to parallelize, but I can then also create a draft PR with the Failure message, so that I get feedback if I am correct that they can not be parallelized and then adjust everything to mark them as unparallelizable :)

@orbeckst
Copy link
Member

orbeckst commented Oct 7, 2024

@talagayev can you please resolve conflicts?

@yuxuanzhuang can you please squash-merge when you're happy (please also write a good log messages — see, for example, PR #4718 )

@talagayev
Copy link
Member Author

@orbeckst @yuxuanzhuang sorry for the late replies, I am on holiday currently till mid of next week and thus have only sometimes access to the laptop 🙈

@yuxuanzhuang should I do anything else or now that you merged the development branch into the PR branch it is ready to be merged? :)

@yuxuanzhuang
Copy link
Contributor

@talagayev I’ll take care of it, no worries! You’ve been incredibly helpful in moving this forward, no need to apologize!

@talagayev
Copy link
Member Author

@yuxuanzhuang Nice, thanks to you too for looking into the PR's and helping with them :)

I would then create the couple of PR's as drafts today for some functions where I am not sure if they can be parallelized so that you and the other core devs can take a look into it and give your opinions, since I have them prepared locally and am at the laptop right now.

Happy to help :)

@yuxuanzhuang yuxuanzhuang merged commit 740e74e into MDAnalysis:develop Oct 7, 2024
22 of 23 checks passed
lilyminium pushed a commit to lilyminium/mdanalysis that referenced this pull request Oct 19, 2024
…alysis#4721)

- Fixes MDAnalysis#4674 
- Parallelization of the backend support to the class DSSP in dssp.py
- Addition of parallelization tests in test_dssp.py and fixtures in conftest.py
- Updated Changelog

---------

Co-authored-by: Yuxuan Zhuang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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