Hbond update #3848

jaclark5 · 2022-09-21T22:21:33Z

Fixes #3847

Changes made in this Pull Request:

Added warning when distance or angle cutoffs produce zero hbonds to clarify a returned value of NaN
Guess_donor uses bonding information when available since it calls hydrogen types anyway
If resnames and names are not available, atom types are used. This is inclusive to LAMMPS users.
Change min max constraints to match inclusive language in the documentation.

PR Checklist

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

jaclark5 · 2022-09-21T22:22:45Z

I expect these changes will spur discussion so I didn't bother with the tests yet.

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

richardjgowers

I think these changes don't look that controversial. Changing the output schema/columns might break something in the output, but I guess that will shake out when tests are added.

jaclark5 · 2022-09-22T12:49:04Z

It won't break the output. I've been using it with success for quite some time. I actually forgot I made some of these changes but I want to merge them and then stay current with the package.

codecov · 2022-09-22T13:31:38Z

Codecov Report

Base: 94.34% // Head: 94.38% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (0a16b13) compared to base (6bc52ec).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3848      +/-   ##
===========================================
+ Coverage    94.34%   94.38%   +0.03%     
===========================================
  Files          194      194              
  Lines        25226    25242      +16     
  Branches      3402     3406       +4     
===========================================
+ Hits         23800    23825      +25     
+ Misses        1377     1368       -9     
  Partials        49       49

Impacted Files	Coverage Δ
...DAnalysis/analysis/hydrogenbonds/hbond_analysis.py	`98.85% <100.00%> (+0.11%)`	⬆️
package/MDAnalysis/due.py	`76.66% <0.00%> (+29.99%)`	⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

hmacdope

looking really good! few comments.

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

package/doc/sphinx/source/documentation_pages/references.rst

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

…find hydrogen atoms.

.gitignore

package/CHANGELOG

IAlibay · 2022-10-12T09:51:01Z

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

+        else:
+            hydrogens_list = np.unique(
+                [
+                    'type {}'.format(tp) for tp in hydrogens_ag.types


I'm only quickly going through this so I might be missing something obvious - doesn't this assume that all types are the same? Wouldn't this be a problem if you end up with a type where some of the atoms are outside the bounds of the allowable charges but not others?

My recommendation here would be to default to a unique attribute, like the ix value, instead of using something that's broad and could pick up stuff outside the intended atomgroup selection.

@IAlibay In most cases an atom type in LAMMPS is all identical with the exception of forcefields such as ReaxFF and COMB. Although you are right that LAMMPS allows for atoms to have individual charges so this could be an issue as forcefields develop...

Here's the issue, these structures look for a list of atom selection strings that represent categories, so if this was on an index basis, the list would be very long, and the resulting output would be useless (representing a single interaction). One could have this structure instead search the atoms of each type and make a long string that specifies the specific indices that meet the charge criteria. However, this string will be unnecessary computational time in 99% of cases, and in the 1% I would argue it would skew the results as these many-body forcefields will continue to fluctuate in charge throughout the simulation, as they are supposed to. I would argue counting "low" or "high" charges in that case wouldn't disrupt the intention of the calculation, it would result in a broken hydrogen bond with a more "realistic" decay time due to local atom environments.

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

testsuite/MDAnalysisTests/analysis/test_hydrogenbonds_analysis.py

IAlibay

Just the one thing and one issue to raise please.

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

testsuite/MDAnalysisTests/analysis/test_hydrogenbonds_analysis.py

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

orbeckst

Minor doc changes + pep 8, please.

I am basically ok with the use of types, given that what we do is messy either way... see comment.

package/CHANGELOG

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

orbeckst

Thanks for addressing my previous requests. I found a few doc things and suggestions for simplifying code.

I am waiting for @IAlibay (and anyone else) to weigh in on the __group_categories() function.

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

orbeckst

Thank you for addressing my requested changes. LGTM!

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

hmacdope

Just a few minor things.

package/CHANGELOG

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

hmacdope

Other than raising the issue mentioned, all the comments I had have been addressed.

IAlibay · 2022-11-30T00:25:42Z

@jaclark5 I've not forgotten about this, it's at the top of my review list over the next few days.

IAlibay · 2022-12-02T23:30:08Z

/azp run

azure-pipelines · 2022-12-02T23:30:17Z

Azure Pipelines successfully started running 1 pipeline(s).

IAlibay

lgtm, thanks for doing this @jaclark5. Sorry it took so long to re-review, but at least it'll make it for the 2.4.0 release 🎉 !

IAlibay · 2022-12-07T11:34:43Z

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

+                '(resname {} and name {})'.format(r, 
+                    p) for r, p in zip(group.resnames, group.names)
+            ])
+        else:


I still think we should have the option to choose whether or not to default to types here, however for the sake of getting this done I'm going to let it go as-is here, and raise a separate issue to deal with later.

IAlibay · 2022-12-07T11:40:13Z

testsuite/MDAnalysisTests/analysis/test_hydrogenbonds_analysis.py

        assert any(warning in rec.getMessage() for rec in caplog.records)


+class TestHydrogenBondAnalysisNoRes(TestHydrogenBondAnalysisIdeal):


Thanks for doing this, avoiding duplicate code as much as possible really helps us avoid future maintenance debt.

orbeckst · 2022-12-07T16:51:32Z

Thank you @jaclark5 , nice work! 🎉

jaclark5 added 3 commits September 21, 2022 17:46

Added warning explain hbond nan

c47513d

Allow atom types for hbond, inclusive to LAMMPS

0140005

Use bond info if available for guess_donor

340cd3f

github-actions bot added the Component-Analysis label Sep 21, 2022

richardjgowers reviewed Sep 22, 2022

View reviewed changes

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py Outdated Show resolved Hide resolved

richardjgowers reviewed Sep 22, 2022

View reviewed changes

hmacdope requested changes Sep 26, 2022

View reviewed changes

jaclark5 added 3 commits September 27, 2022 09:28

Added hbond PR tests and CHANGELOG with bug fixes

d1a7182

Hydrogen Bonding: Update docs

f7f8bf5

Amended hbond test for full coverage, added _get_dh_pairs ability to …

299b898

…find hydrogen atoms.

jaclark5 force-pushed the hbond_update branch 2 times, most recently from 870e90d to 5f9fbab Compare September 29, 2022 00:56

hbond_analysis complete codecov

bd95900

jaclark5 force-pushed the hbond_update branch from 5f9fbab to bd95900 Compare September 29, 2022 01:48

Adjusted hbond doc strings for max_*/min_* bounds

5d27895

jaclark5 marked this pull request as ready for review September 29, 2022 21:51

Merge branch 'develop' into hbond_update

7903c4f

jaclark5 requested a review from hmacdope October 11, 2022 21:21

IAlibay requested changes Oct 12, 2022

View reviewed changes

Incorporate reviewer comments

7af74d8

jaclark5 requested review from IAlibay and removed request for hmacdope October 20, 2022 21:30

Merge branch 'develop' into hbond_update

9966f74

IAlibay requested changes Oct 26, 2022

View reviewed changes

jaclark5 and others added 3 commits November 4, 2022 10:44

Review comment integration

77af51e

bug fix

4ae3b7b

Merge branch 'develop' into hbond_update

a61b56e

Updated tests

333ef6b

jaclark5 force-pushed the hbond_update branch 2 times, most recently from 060e6bb to 333ef6b Compare November 4, 2022 22:40

Remove unused line

049c0ee

orbeckst requested changes Nov 5, 2022

View reviewed changes

Integrate reviewer comments

2788935

jaclark5 force-pushed the hbond_update branch from f7185a1 to 2788935 Compare November 5, 2022 15:33

Add test for full coverage

a607605

orbeckst requested changes Nov 5, 2022

View reviewed changes

Integrate reviewer comments

50dd832

orbeckst approved these changes Nov 6, 2022

View reviewed changes

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py Show resolved Hide resolved

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py Show resolved Hide resolved

hmacdope requested changes Nov 9, 2022

View reviewed changes

orbeckst and others added 2 commits November 11, 2022 14:55

Merge branch 'develop' into hbond_update

8cf40a1

Update test and docs

d5fbe48

hmacdope approved these changes Nov 20, 2022

View reviewed changes

jaclark5 mentioned this pull request Nov 21, 2022

modernize HBA to use AG as objects internally instead of selection strings #3933

Open

jaclark5 requested a review from IAlibay November 21, 2022 22:32

Merge branch 'develop' into hbond_update

0a16b13

IAlibay added this to the 2.4.0 milestone Nov 30, 2022

IAlibay mentioned this pull request Dec 7, 2022

Allow for optional types grouping in hbond code (related to #3933) #3949

Open

IAlibay approved these changes Dec 7, 2022

View reviewed changes

IAlibay merged commit 3712439 into MDAnalysis:develop Dec 7, 2022

IAlibay added the enhancement label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hbond update #3848

Hbond update #3848

jaclark5 commented Sep 21, 2022 •

edited

Loading

jaclark5 commented Sep 21, 2022

richardjgowers left a comment

jaclark5 commented Sep 22, 2022

codecov bot commented Sep 22, 2022 •

edited

Loading

hmacdope left a comment

IAlibay Oct 12, 2022

jaclark5 Oct 12, 2022 •

edited

Loading

IAlibay left a comment

orbeckst left a comment

orbeckst left a comment

orbeckst left a comment

hmacdope left a comment

hmacdope left a comment

IAlibay commented Nov 30, 2022

IAlibay commented Dec 2, 2022

azure-pipelines bot commented Dec 2, 2022

IAlibay left a comment

IAlibay Dec 7, 2022

IAlibay Dec 7, 2022

orbeckst commented Dec 7, 2022

		assert any(warning in rec.getMessage() for rec in caplog.records)


		class TestHydrogenBondAnalysisNoRes(TestHydrogenBondAnalysisIdeal):

Hbond update #3848

Hbond update #3848

Conversation

jaclark5 commented Sep 21, 2022 • edited Loading

PR Checklist

jaclark5 commented Sep 21, 2022

richardjgowers left a comment

Choose a reason for hiding this comment

jaclark5 commented Sep 22, 2022

codecov bot commented Sep 22, 2022 • edited Loading

Codecov Report

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay Oct 12, 2022

Choose a reason for hiding this comment

jaclark5 Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

IAlibay commented Nov 30, 2022

IAlibay commented Dec 2, 2022

azure-pipelines bot commented Dec 2, 2022

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Dec 7, 2022

Choose a reason for hiding this comment

IAlibay Dec 7, 2022

Choose a reason for hiding this comment

orbeckst commented Dec 7, 2022

jaclark5 commented Sep 21, 2022 •

edited

Loading

codecov bot commented Sep 22, 2022 •

edited

Loading

jaclark5 Oct 12, 2022 •

edited

Loading