Closed orbeckst closed 4 years ago
@xiki-tempula you said about the new proposed HydrogenBondAnalysis (PR #2237 by @p-j-smith ) https://github.com/MDAnalysis/mdanalysis/pull/2237#issuecomment-480543288
The code looks really nice, the capped distance is a very neat way of finding the atoms compared with the old neighbour search. I will try to use capped_distance in the water bridge we well. I guess the only problem is that the interface and the outputs are very different from the previous one. Personally, I hate the original representation as well.
Do you think there is a way that you can build/refactor WaterBridgeAnalysis on the new class?
I think we are starting a discussion about what the API of a new HydrogenBondAnalysis ought to look like so things would likely change. But you would think that you could – in principle – work with the new class? And do you think that would actually have the time to do it if necessary?
@orbeckst I guess the major obstacle is the underlying data structure. In wba, the data is stored as a network to represent the nature of the water network, so the analysis part acts upon the underlying network data structure. The other though a bit trivial problem is the amount of redundancy removed by combing two classes. If expressed in simple pseudocode, hba works as
for frame in frames:
find_hb(sele1, sele2)
whereas wba works as:
for frame in frames:
find_hb(sele1, sele2)
find_hb(sele1, water1)
for water_no in number_of_water:
find_hb(water[water_no], sele2)
find_hb(water[water_no], water[water_no+1])
So the only common part is find_hb(sele1, sele2), which is only 3 lines in the current PR.
Update:
MDAnalysis.analysis.hydrogenbonds.hbond_analysis
so that we can keep the old version in MDAnalysis.analysis.hbonds.hbond_analysis
and slowly move it into deprecated and legacy status (by 1.0?).hbonds.hbond_analysis
around for a while (with its tests) but won't continue developing it and will strive to replace it with the new version within MDAnalysis (e.g., for WaterBridgeAnalysis, PR #2087).The PR #2087 is fully compatible with the old hbond_analysis and has passed all the test. I wonder if it is possible to review the PR, please? Thank you.
As part of this waterdynamics.HydrogenBondLifetimes
should be refactored to use hydrogenbonds.HydrogenBondAnalysis
and/or turned into a subclass. Edit: #2547
This issue was fixed by PR #2237.
Anything else can go into their own issues.
Is your feature request related to a problem? Please describe. The
analysis.hbonds.hbond_analysis.HydrogenBondAnalysis
class is very useful and widely used but the code is messy and old and thus hard to maintain, to debug (see e.g. #1687), or to extend. It also does _not follow the Analysis API_. Performance is also not stellar (e.g. does not usecapped_distances()
)Describe the solution you'd like Refactor/rewrite, ideally without breaking the API, which includes
HydrogenBondAnalysis
count_by_time()
,count_by_type()
,timesteps_by_type()
The data structures should be clean, see #2177 for a discussion.
The class should implement the Analysis API ("Bauhaus") from #719.
Describe alternatives you've considered
Current work
Currently there are two PRs with different solutions