Moves hbond_autocorrel and removes old hbond code

[IAlibay]

Fixes #2739
Fixes #3259

Changes made in this Pull Request:

• Removes old hbond code
• Add warning for upcoming move of hbond_autocorrel to hydrogenbonds (for consistency).

PR Checklist

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

[pep8speaks]

Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py:

Line 23:80: E501 line too long (100 > 79 characters)
Line 62:80: E501 line too long (80 > 79 characters)
Line 67:80: E501 line too long (82 > 79 characters)

• In the file package/MDAnalysis/analysis/hydrogenbonds/hbond_autocorrel.py:

Line 24:80: E501 line too long (91 > 79 characters)
Line 49:80: E501 line too long (94 > 79 characters)
Line 78:80: E501 line too long (87 > 79 characters)
Line 225:1: E124 closing bracket does not match visual indentation
Line 306:9: E265 block comment should start with '# '
Line 306:80: E501 line too long (92 > 79 characters)
Line 395:56: E127 continuation line over-indented for visual indent
Line 396:56: E127 continuation line over-indented for visual indent
Line 397:43: E127 continuation line over-indented for visual indent
Line 431:80: E501 line too long (102 > 79 characters)
Line 435:49: E225 missing whitespace around operator
Line 440:9: E303 too many blank lines (2)
Line 449:36: E127 continuation line over-indented for visual indent
Line 572:80: E501 line too long (91 > 79 characters)
Line 581:17: E122 continuation line missing indentation or outdented

• In the file package/MDAnalysis/analysis/hydrogenbonds/wbridge_analysis.py:

Line 24:80: E501 line too long (90 > 79 characters)

• In the file testsuite/MDAnalysisTests/analysis/test_hydrogenbondautocorrel_deprecated.py:

Line 39:1: E302 expected 2 blank lines, found 1
Line 43:1: E302 expected 2 blank lines, found 1
Line 68:9: E124 closing bracket does not match visual indentation
Line 73:23: E201 whitespace after '['
Line 73:33: E203 whitespace before ','
Line 87:9: E124 closing bracket does not match visual indentation
Line 92:23: E201 whitespace after '['
Line 92:33: E203 whitespace before ','
Line 98:5: E303 too many blank lines (2)
Line 105:9: E124 closing bracket does not match visual indentation
Line 110:23: E201 whitespace after '['
Line 110:33: E203 whitespace before ','
Line 116:5: E303 too many blank lines (2)
Line 124:9: E124 closing bracket does not match visual indentation
Line 129:23: E201 whitespace after '['
Line 129:33: E203 whitespace before ','
Line 143:9: E124 closing bracket does not match visual indentation
Line 148:23: E201 whitespace after '['
Line 148:33: E203 whitespace before ','
Line 162:9: E124 closing bracket does not match visual indentation
Line 187:9: E124 closing bracket does not match visual indentation
Line 196:80: E501 line too long (85 > 79 characters)
Line 218:9: E124 closing bracket does not match visual indentation
Line 232:9: E124 closing bracket does not match visual indentation
Line 244:9: E124 closing bracket does not match visual indentation
Line 253:9: E124 closing bracket does not match visual indentation
Line 258:80: E501 line too long (83 > 79 characters)
Line 269:9: E122 continuation line missing indentation or outdented
Line 278:9: E124 closing bracket does not match visual indentation
Line 284:80: E501 line too long (86 > 79 characters)
Line 297:1: E303 too many blank lines (3)
Line 319:80: E501 line too long (85 > 79 characters)
Line 331:1: W391 blank line at end of file

Comment last updated at 2021-05-06 06:54:17 UTC

[IAlibay]

I realise we usually create stubs so that folks can change workflows ahead of time, however the removal of history for hbond_autocorrel bothered me. Since all it means to users is that they have to change their import, can we just do this as a warned move without moving things ahead of time? (worst case I create a stub in hydrogenbonds that imports hbond.hbond_autocorrel, but that makes warnings messy)

Edit: please ignore me, this doesn't fix the history problem, we lose history as soon as we move the file...

[orbeckst]

At the moment the test_api tests fail because there's nothing in hydrogenbonds. I'll push a change with the moved file and stubs in the old place. If you don't like it, revert it.

[IAlibay]

@orbeckst please go ahead, I was going to try to complete this tomorrow, but any push along is appreciated :)

[orbeckst]

I am also fixing #3259 because I have to edit the docs in wbridge_analysis anyway.

[codecov]

Codecov Report

Merging #3258 (6a8dccd) into develop (903ae4c) will increase coverage by 0.10%.
The diff coverage is 95.20%.

@@             Coverage Diff             @@
##           develop    #3258      +/-   ##
===========================================
+ Coverage    92.91%   93.02%   +0.10%     
===========================================
  Files          172      172              
  Lines        23033    22694     -339     
  Branches      3266     3193      -73     
===========================================
- Hits         21402    21110     -292     
+ Misses        1581     1534      -47     
  Partials        50       50              

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/hbonds/__init__.py	100.00% <ø> (ø)
...nalysis/analysis/hydrogenbonds/hbond_autocorrel.py	94.87% <94.87%> (ø)
...age/MDAnalysis/analysis/hbonds/hbond_autocorrel.py	100.00% <100.00%> (+5.12%)	⬆️
...kage/MDAnalysis/analysis/hydrogenbonds/__init__.py	100.00% <100.00%> (ø)
...nalysis/analysis/hydrogenbonds/wbridge_analysis.py	97.41% <100.00%> (ø)
package/MDAnalysis/topology/guessers.py	99.22% <0.00%> (-0.78%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 903ae4c...6a8dccd. Read the comment docs.

[orbeckst]

I am ok with your and my changes... if you or anyone else could have a quick look then that would be good.

@IAlibay I am leaving it to you to merge/request changes.

[IAlibay]

@orbeckst is there any point in having most of the hbond_autocorrel tests duplicated here? They are pretty expensive iirc, if you're happy with it, we can just move these two warning tests over to the other hbond_autocorrel tests?

[orbeckst]

Tests run in seconds and I wanted to make sure that the stubs properly process arguments (I wasn’t 100% sure if I could use the deprecate decorator on a class like that..). For 2.0.0 I’d leave the full tests (no harm) and then we can slim it down later. I didn’t quite know which ones to remove but still test basic functionality.

…

Am 5/6/21 um 07:59 schrieb Irfan Alibay ***@***.***>:  @IAlibay commented on this pull request. In testsuite/MDAnalysisTests/analysis/test_hydrogenbondautocorrel_deprecated.py: > + D = hbonds.find_hydrogen_donors(H) + + assert len(H) == len(D) + # check each O is bonded to the corresponding H + for h_atom, o_atom in zip(H, D): + assert o_atom in h_atom.bonded_atoms + + +def test_donors_nobonds(): + u = mda.Universe(XYZ_mini) + + with pytest.raises(mda.NoDataError): + hbonds.find_hydrogen_donors(u.atoms) + + +def test_find_hydrogen_donors_deprecation_warning(u_water): @orbeckst is there any point in having most of the hbond_autocorrel tests duplicated here? They are pretty expensive iirc, if you're happy with it, we can just move these two warning tests over to the other hbond_autocorrel tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[IAlibay]

@orbeckst - ok that's fair. I'm happy with this (except from being sad that we will lose git history for hbond_autocorrel) so I'll squash-merge.