Transport Properties using Onsager Formalism #56
Conversation
|
Hello @rohithsrinivaas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-12-02 05:30:19 UTC |
rohithsrinivaas
changed the title
|
Would you mind linting this with something like black before I review? |
|
@orionarcher I have used black to lint as suggested. Kindly review the code and provide your comments |
There was a problem hiding this comment.
Thank you @rohithsrinivaas! A of really awesome work here and I'm excited to have it in the package.
A few suggestions
- Put both conductivity implementations in the same file and just call it
conductivity.py - change
diffusion_msd.pytodiffusion.py - The function signatures need to be cleaned up in several places and the
resultsobject documented. I've mentioned this in my comments - Write tests for the new functionality. For that, I'd suggest adding a small ionic system to the test data . Just need to make sure everything can run end to end, it doesn't need to be scientifically rigorous, I trust you've done that already.
solvation_analysiscould be a good template for this. See the datafile and the contest. - There are more suggestions in the comments that I won't repeat here.
|
|
||
| def plot_acf(self): | ||
| """ | ||
| Plot the mead square displacement vs time and the fit region in the log-log scale |
There was a problem hiding this comment.
| Plot the mead square displacement vs time and the fit region in the log-log scale | |
| Plot the mean square displacement vs time and the fit region in the log-log scale |
| allatomgroup: "AtomGroup", | ||
| cationgroup_query: str, | ||
| aniongroup_query: str, |
There was a problem hiding this comment.
I would not use queries for selecting anions and cations, just provide them directly
| allatomgroup: "AtomGroup", | |
| cationgroup_query: str, | |
| aniongroup_query: str, | |
| cations: "AtomGroup", | |
| anions: "AtomGroup", |
| cation_num_atoms_per_species=int, | ||
| anion_num_atoms_per_species=int, | ||
| cation_mass_per_species=float, | ||
| anion_mass_per_species=float, |
There was a problem hiding this comment.
X_num_atoms_per_species can be calculated internally and masses are available at atoms.masses, no need to include any of these as kwargs
cations = # define cation atom group
cation_num_atoms_per_species = int(len(cations) / len(cations.residues))
# same for anions
cation_mass_per_species = | Class to calculate the diffusion_coefficient of a species. | ||
| Note that the slope of the mean square displacement provides | ||
| the diffusion coeffiecient. This implementation uses FFT to compute MSD faster | ||
| as explained here: https://stackoverflow.com/questions/34222272/computing-mean-square-displacement-using-python-and-fft | ||
| Since we are using FFT, the sampling frequency (or) dump frequency of the simulation trajectory | ||
| plays a critical role in the accuracy of the result. | ||
|
|
||
| Particle velocities are required to calculate the viscosity function. Thus | ||
| you are limited to MDA trajectories that contain velocity information, e.g. | ||
| GROMACS .trr files, H5MD files, etc. See the MDAnalysis documentation: | ||
| https://docs.mdanalysis.org/stable/documentation_pages/coordinates/init.html#writers. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| atomgroup : AtomGroup | ||
| An MDAnalysis :class:`~MDAnalysis.core.groups.AtomGroup`. | ||
| Note that :class:`UpdatingAtomGroup` instances are not accepted. | ||
| temp_avg : float (optional, default 300) | ||
| Average temperature over the course of the simulation, in Kelvin. | ||
| dim_type : {'xyz', 'xy', 'yz', 'xz', 'x', 'y', 'z'} | ||
| Desired dimensions to be included in the viscosity calculation. | ||
| Defaults to 'xyz'. | ||
| linear_fit_window : tuple of int (optional) | ||
| A tuple of two integers specifying the start and end lag-time for | ||
| the linear fit of the viscosity function. If not provided, the | ||
| linear fit is not performed and viscosity is not calculated. |
There was a problem hiding this comment.
Indentation is funky here, look at another class for an example
| cation_charge=int, | ||
| anion_charge=int, |
There was a problem hiding this comment.
Include defaults
| cation_charge=int, | |
| anion_charge=int, | |
| cation_charge: int=1, | |
| anion_charge: int=-1, |
| results.timeseries : :class:`numpy.ndarray` | ||
| The averaged viscosity function over all the particles with respect | ||
| to lag-time. Obtained after calling :meth:`ViscosityHelfand.run` | ||
| results.visc_by_particle : :class:`numpy.ndarray` | ||
| The viscosity function of each individual particle with respect to | ||
| lag-time. | ||
| results.viscosity : float | ||
| The viscosity coefficient of the solvent. The argument | ||
| `linear_fit_window` must be provided to calculate this to | ||
| avoid misinterpretation of the viscosity function. |
There was a problem hiding this comment.
I don't think there are correct for this class
| dim_fac : int | ||
| Dimensionality :math:`d` of the viscosity computation. |
| self.results.cation_transference_com = ( | ||
| (self.cation_charge**2) * lij_cation_cation | ||
| + (self.cation_charge * self.anion_charge) * lij_cation_anion | ||
| ) / cond | ||
| self.results.anion_transference_com = ( | ||
| (self.anion_charge**2) * lij_anion_anion | ||
| + (self.cation_charge * self.anion_charge) * lij_cation_anion | ||
| ) / cond | ||
| self.results.cation_transference_solvent = ( | ||
| self.results.cation_transference_com - anion_mass_fraction | ||
| ) / solvent_fraction | ||
| self.results.anion_transference_solvent = ( | ||
| self.results.anion_transference_com - cation_mass_fraction | ||
| ) / solvent_fraction |
There was a problem hiding this comment.
The contents of results should be documented here. It's not clear to me what the difference between the _solvent and _com quantities is?
| results.timeseries : :class:`numpy.ndarray` | ||
| The averaged viscosity function over all the particles with respect | ||
| to lag-time. Obtained after calling :meth:`ViscosityHelfand.run` | ||
| results.visc_by_particle : :class:`numpy.ndarray` | ||
| The viscosity function of each individual particle with respect to | ||
| lag-time. | ||
| results.viscosity : float | ||
| The viscosity coefficient of the solvent. The argument | ||
| `linear_fit_window` must be provided to calculate this to | ||
| avoid misinterpretation of the viscosity function. |
There was a problem hiding this comment.
Document results signature
| allatomgroup: "AtomGroup", | ||
| atomgroup_query: str, |
There was a problem hiding this comment.
Not clear why we need a query here?
| allatomgroup: "AtomGroup", | |
| atomgroup_query: str, | |
| atomgroup: "AtomGroup", |
Would need to be changed below too.
Fixes #44 #45 #18
Changes made in this Pull Request:
References:
https://github.com/kdfong/transport-coefficients-MSD
https://github.com/kdfong/transport-coefficients
https://pubs.acs.org/doi/10.1021/acs.macromol.0c02001
https://arxiv.org/abs/2006.16164
PR Checklist