-
Notifications
You must be signed in to change notification settings - Fork 663
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
Conversation
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
Hello @talagayev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-07 17:46:24 UTC |
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.
Please have a look at the Please note: The |
adjusting dssp.py for PEP
Adjusting for PEP
black adjustment
removed whitespaces
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Co-authored-by: Yuxuan Zhuang <[email protected]>
I would have then also have a quick question:
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? |
@talagayev it would help to see the full backtrace for the error, otherwise it's a bit hard to help.
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 |
@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 ) |
@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? :) |
@talagayev I’ll take care of it, no worries! You’ve been incredibly helpful in moving this forward, no need to apologize! |
@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 :) |
…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]>
Fixes #4674
Changes made in this Pull Request:
DSSP
indssp.py
versionchanged
to mention the addition of the parallelizationtest_dssp.py
and fixtures inconftest.py
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4721.org.readthedocs.build/en/4721/