Overhaul forcefield parameters for ions #15
Conversation
reported as kJ/mol; need to convert to kcal/mol
-Went back to original papers -checked and converted all values -cross-checked previous data entry -moved ion parameters into an .xlsx file instead of text files -reworked call signature of get_ion In the future it would be preferable to store the ion data as a .json instead of .xlsx. However pandas read_json does not support MultiIndex very well.
reported as kJ/mol; need to convert to kcal/mol
-Went back to original papers -checked and converted all values -cross-checked previous data entry -moved ion parameters into an .xlsx file instead of text files -reworked call signature of get_ion In the future it would be preferable to store the ion data as a .json instead of .xlsx. However pandas read_json does not support MultiIndex very well.
|
@rkingsbury Thank! I have resolved the version conflict by importing Literal from typing_extensions |
There was a problem hiding this comment.
@rkingsbury I have also suggested code for specifying mixing rules. Could you please see if you are good with that? Thanks! Some more thought: will it work if we allow users to only specify parameter_set = "jj" without specifying water_model = "tip4p" in order to retrieve the "jj" parameters?
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
Co-authored-by: Tingzheng Hou <thou@berkeley.edu>
This is a good point @htz1992213 ; I hadn't though about the fact that one might want to use some of these ion parameters with solvents other than water. I think what I'll do is reverse the kwargs, so that |
See pymatgen #2287 materialsproject/pymatgen#2287 The data file is now keyed by Ion.reduced_formula, meaning all ions have charges appended in brackets, with explicit magnitudes e.g. `Li[+1]`
|
@rkingsbury Sure, thanks! And I think the auto selection of recommended |
See my changes @htz1992213. Could you add a test of the mixing rule functionality? |
|
OK, there is a test failure which is due to a recent change in pymatgen. See materialsproject/pymatgen#2287. This has not been released yet so I'm not able to update the Previously, the behavior of |
Hi @rkingsbury, I have added tests. I also restored your previous auto-selection of parameter set from a given water model. I think that is a pretty cool feature. Could you please see if that looks good? |
Looks good to me, thanks @htz1992213 ! I guess we'll need to wait for the next pymatgen release in order to merge, since the ion data requires the changes to |
|
@rkingsbury Merged! Thanks! |
Summary
Overhaul of the Lennard Jones parameters for ions. Comments / feedback welcome.
IonLJParametersfor storing the parameters in mdgoIonLJParametersand store inions_lj_params.jsonget_ionsso that the user specifies the ion first, followed by the water model, and then optionally the ion parameter setAdditional dependencies introduced (if any)
None
TODO (if any)
get_waterfor consistency with these changescombining_rulesattribute ofIonLJDatato enforce some kind of checking when mixing water and ion modelsIonLJDatafor the water modelsChecklist
Work-in-progress pull requests are encouraged. Please put [WIP]
in the pull request title.
Before a pull request can be merged, the following items must be checked:
is to run the following in the correct sequence on your local machine. Start with running
black on your new code. This will automatically reformat
your code to PEP8 conventions and removes most issues. Then run
pycodestyle, followed by
flake8.
Run pydocstyle on your code.