Hbond analysis update

[yickysan]

Fixes #3933

Changes made in this Pull Request:

• Changed the guess_hydrogens, guess_donors and guess_acceptors methods to use ag.atoms instead of ag.select.atoms("all") if a selection string isn't given.

PR Checklist

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

---

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

[github-actions]

Linter Bot Results:

Hi @yickysan! 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	⚠️ Possible failure
testsuite	✅ Passed

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/4582888314/jobs/8093390778

---

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

[codecov]

Codecov Report

Patch coverage: 61.90% and project coverage change: -0.03 ⚠️

Comparison is base (bdb1352) 93.59% compared to head (bb25dcd) 93.56%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4107      +/-   ##
===========================================
- Coverage    93.59%   93.56%   -0.03%     
===========================================
  Files          192      192              
  Lines        25133    25144      +11     
  Branches      4056     4060       +4     
===========================================
+ Hits         23523    23526       +3     
- Misses        1092     1097       +5     
- Partials       518      521       +3     

Impacted Files	Coverage Δ
...DAnalysis/analysis/hydrogenbonds/hbond_analysis.py	93.01% <61.90%> (-4.14%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[RMeli]

Hi @yickysan, thanks for you contribution. It looks like in this PR you also have the changes that you did for PR #4090. Could you please remove/revert them?

---

I see you are correctly working on a specific branch for this fix (yickysan:hbond-analysis-update) which is great. You probably created this branch from yickysan:numpy.testing-function-change, instead of creating it from develop and that's likely why you have both changes in this branch.

[yickysan]

Hi @yickysan, thanks for you contribution. It looks like in this PR you also have the changes that you did for PR #4090. Could you please remove/revert them?

I see you are correctly working on a specific branch for this fix (yickysan:hbond-analysis-update) which is great. You probably created this branch from yickysan:numpy.testing-function-change, instead of creating it from develop and that's likely why you have both changes in this branch.

Hello,
Thanks for pointing it out to me. I have deleted the numpy.testing-function-change-branch. leaving only the hbond-analysis-update.

[RMeli]

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).

You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

• 0d7bfd4
• 9c11631
• 4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

[yickysan]

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).

You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

• 0d7bfd4
• 9c11631
• 4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

I am currently reverting these commits as well.

[yickysan]

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).

You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

• 0d7bfd4
• 9c11631
• 4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

Please I need your help. In an attempt to revert the commits I have accidentally deleted some files. Anyway you can help me through this?

@yickysan as you can see from the files changed, deleting the yickysan:numpy.testing-function-change branch (or closing PR #4090) does not solve the issue. This is because the changes have been included in the yickysan:hbond-analysis-update branch as well (the likely explanation of how this happened is in the previous message).
You will need to revert the changes in this branch. You can probably use git revert to revert the commit(s) related to the other branch. More specifically you need to revert the following commit (please double check!):

• 0d7bfd4
• 9c11631
• 4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

I am currently reverting these commits as well.

Hello is it possible to close this pull requests while I make another one? I think in my attempt to revert the other commits I might have broken something.

• 4086def

(These are the commits that you also had in the other PR (see MDAnalysis/pull/4090/commits and the commit message appears to be related to the tests).

I am currently re

[RMeli]

How did you delete the file? Were you using git revert? Since you deleted the whole testsuite/MDAnalysisTests/analysis/test_align.py file, I think the simplest fix here would be to download such file from the develop branch in the MDAnalysis repo and add it back to your repo. This way, you should have the file back, and also revert your commits regarding the file.

Don't be discouraged by these difficulties with git. They are quite normal and get better with practice: xkcd.com/1597 =)

[yickysan]

How did you delete the file? Were you using git revert? Since you deleted the whole testsuite/MDAnalysisTests/analysis/test_align.py file, I think the simplest fix here would be to download such file from the develop branch in the MDAnalysis repo and add it back to your repo. This way, you should have the file back, and also revert your commits regarding the file.

Don't be discouraged by these difficulties with git. They are quite normal and get better with practice: xkcd.com/1597 =)

I think I must have accidentally deleted the file trying to handle a merge conflict. I used git reset and not revert. I have redownloaded the file and added it back to my local repo.

[RMeli]

Make sure to commit and push, so that the file is restored in this PR as well. If everything goes well, it should disappear from the "files changed" tab.

You can read here about the difference between reset and revert: https://git-scm.com/docs/git#_reset_restore_and_revert The latter is safer because it does not change the commit history.

[RMeli]

@yickysan Looks like you removed all the changes, including the ones related to this PR? There is nothing to review at the moment. (I'm on mobile, apologies if I missed something ing.)

[RMeli]

You can use "git status" and "git diff" locally to check what's going and in which state is your (local) repo.

[yickysan]

@yickysan Looks like you removed all the changes, including the ones related to this PR? There is nothing to review at the moment. (I'm on mobile, apologies if I missed something ing.)

Yes the revert I made removed the file related to this pull request but I am adding it back right now.

[yickysan]

You can use "git status" and "git diff" locally to check what's going and in which state is your (local) repo.

Hello @RMeli I added back the commit related to this PR. It failed the codecov tests. Is there a way to fix this?

[IAlibay]

I'm not sure I understand the intent behind these changes, how is this intended to change the usability of the hbond analysis class? Maybe if you can provide a quick walked through example of how a user might help review.

re coverage: this is happening because you don't have any tests that cover the new code branches that were introduced in this PR. To avoid them, you would need to add tests that try out the new input method you've introduced.

[IAlibay]

Ratther than doing a completely new code branch for each option, why not just do this selection solely in an if/else and leaving everything else the same?

[yickysan]

I'm not sure I understand the intent behind these changes, how is this intended to change the usability of the hbond analysis class? Maybe if you can provide a quick walked through example of how a user might help review.

re coverage: this is happening because you don't have any tests that cover the new code branches that were introduced in this PR. To avoid them, you would need to add tests that try out the new input method you've introduced.

So it doesn't change the usability of the hbondanalysis class. It just changes the guess_donors, guess_hydrogens methods, etc to call u.atoms if a selection string isn't given instead of calling u.select_atoms("all")