MDAnalysis / solvation-analysis

A comprehensive tool for analyzing liquid solvation structure.

https://solvation-analysis.readthedocs.io/en/latest/

GNU General Public License v3.0

46 stars 13 forks source link

Residence times and solute-solvent network calculations #59

Closed orionarcher closed 2 years ago

orionarcher commented 2 years ago

Description

This PR adds an analysis module to calculate residence times and an analysis module to calculate solute-solvent networks.

Todos

Notable points that this PR has either accomplished or will accomplish.

[x] Residence module
[x] Change exponential fit in residence time calculations to 1/e decay point
[x] Networking module
[x] Residence testing
[x] Networking testing
[x] Residence documentation
[x] Networking documentation
[x] Add diluent_composition analysis to Pairing class
[x] Add module selection kwarg to Solution
[x] Add from_solution class method to all analysis classes
[x] Residence and Networking tutorial
[x] add citations for Networking and Residence codes
[x] improve documentation for Residence time
- [x] specify caveats and difference between both implementations

Status

The PR is nearly finished, the main outstanding issues are improved documentation, citations and tutorials.

review-notebook-app[bot] commented 2 years ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

pep8speaks commented 2 years ago

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

In the file solvation_analysis/analysis_library.py:

Line 556:5: E303 too many blank lines (2) Line 635:9: E731 do not assign a lambda expression, use a def Line 641:9: E722 do not use bare 'except' Line 880:17: E131 continuation line unaligned for hanging indent

In the file solvation_analysis/tests/test_analysis_library.py:

Line 13:1: E302 expected 2 blank lines, found 1

Comment last updated at 2022-06-28 19:26:47 UTC

orionarcher commented 2 years ago

New changes implement better documentation for the Residence and Networking classes. Citations, better descriptions of the methods, and general cleanup is still needed.

hmacdope commented 2 years ago

This looks awesome @orioncohen, ping me if / when you want review

orionarcher commented 2 years ago

Thanks! I'm stoked about the features here. I am leveraging some clever pandas calls to use solvation_data as a sparse adjacency matrix.

Once I have testing in place and clean up the docs I'll request a review.

orionarcher commented 2 years ago

Hey @hmacdope, this is nearing completion so if you have some time, would love a review!

One point to consider:

Previously, all analysis_library classes were automatically instantiated in Solution. This was possible because Speciation, Pairing, and Coordination are quite cheap. In contrast, Networking and Residence are quite expensive. I decided to make Speciation, Pairing, and Coordination automatically be added to Solution by default but gave the user the option to customize what analysis modules are instantiated with the analysis_classes kwarg. Curious to hear feedback on that implementation. It was the best way I could think to strike a balance between flexibility and usability.

More documentation and citations are needed but overall I'm happy with how this is shaping up!

codecov[bot] commented 2 years ago

Codecov Report

Merging #59 (d0cca10) into main (e95ff5b) will increase coverage by 2.48%. The diff coverage is 97.14%.

orionarcher commented 2 years ago

Thanks for the review @hmacdope! I believe I have addressed all the points you made. I'm going to make sure the documentation looks good then merge.