mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

Used between to update donors_sel and acceptors_sel

Open Sumit112192 opened this issue 2 years ago • 5 comments

Fixes #4130

Changes made in this Pull Request:

  • I used the between keyword to update the self.donors_sel and self.acceptors_sel keywords to reduce the search space for the capped_distance function.
  • I want to know if my logic is correct so that I can think more in that direction.

PR Checklist

  • [ ] Tests?
  • [ ] Docs?
  • [ ] CHANGELOG updated?
  • [x] Issue raised/referenced?

Developer certificate of origin


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

Sumit112192 avatar Jan 11 '24 14:01 Sumit112192

Linter Bot Results:

Hi @Sumit112192! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

github-actions[bot] avatar Jan 11 '24 14:01 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (846131c) 93.36% compared to head (8eac74a) 93.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##           develop    #4422     +/-   ##
==========================================
  Coverage    93.36%   93.36%             
==========================================
  Files          171      185     +14     
  Lines        21736    22853   +1117     
  Branches      4012     4012             
==========================================
+ Hits         20293    21337   +1044     
- Misses         954     1027     +73     
  Partials       489      489             

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 11 '24 14:01 codecov[bot]

In the hydrogen bond analysis, if the user passed donor_sel, it might cause a pair of results where both hydrogen and donor have the same index, essentially the same hydrogen atom, since we were not putting any min_cutoff constraint in the capped_distance function of _get_dh_pairs() function

Sumit112192 avatar Jan 12 '24 13:01 Sumit112192

Can someone guide me if I am going in the right direction here?

Sumit112192 avatar Jan 29 '24 11:01 Sumit112192

@richardjgowers could you please shepherd this PR? You're the one who originally made the performance improvement suggestion in #4130. Thanks!

orbeckst avatar Feb 10 '24 01:02 orbeckst