-
Notifications
You must be signed in to change notification settings - Fork 696
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
Fixes #4924 #4961
base: develop
Are you sure you want to change the base?
Fixes #4924 #4961
Conversation
… generate_kirchoff: fix residue_index_map generate to avoid use all atoms when select is 'name CA'
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS
as part of this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4961 +/- ##
===========================================
- Coverage 93.42% 93.41% -0.02%
===========================================
Files 177 189 +12
Lines 21859 22925 +1066
Branches 3078 3078
===========================================
+ Hits 20422 21415 +993
- Misses 986 1059 +73
Partials 451 451 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Can you add a new test that shows that the bug has been fixed?
The test should FAIL without your fix and PASS with your fix.
The test should be added to testsuite/MDAnalysisTests/analysis/test_gnm.py.
…alysis - Add test case to verify correct behavior when selecting 'name CA' atoms - Ensure that all residues are included in the analysis, fixing the issue where only residues 0-14 were used - Validate the results against expected eigenvalues and Kirchhoff matrix
… readability - Improved code formatting and line breaks for better readability
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.
Can you please also add an entry to CHANGELOG under Fixes?
Thanks for adding the test. I have to look at it in more detail. Does it make sense that the eigenvalues for the CA case are essentially zero (~1e-16)?
I did not read the original paper on the algorithm, but I tried to optimize the code for speed and studied it in detail. I think it is because we get fewer contacts in a residue with selection Here are the diffrent values with different selections in the test case:
The value increases as the number of atoms increases. This fix also corrects the case in 'backbone'. |
… eigenvalue comparison
- Lowered precision in eigenvalue comparison to accommodate platform-specific results - Added comment about platform-specific results to explain the change in expected values
[… generate_kirchoff: fix residue_index_map generate to avoid use all atoms when select is 'name CA'](fix-bug: package/MDAnalysis/analysis/gnm.py: closeContactGNMAnalysis: generate_kirchoff: fix residue_index_map generate to avoid use all atoms when select is 'name CA')
Fixes #4924
Changes made in this Pull Request:
PR Checklist
package/CHANGELOG
file updated?package/AUTHORS
? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--4961.org.readthedocs.build/en/4961/