Conversation
… generate_kirchoff: fix residue_index_map generate to avoid use all atoms when select is 'name CA'
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
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:
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
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
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
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
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Thanks for adding the tests and answering my questions. I think this now looks good.
|
Thank you for your first contribution!! |
2 participants
[… 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/CHANGELOGfile 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/