Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change ion defaults #32

Merged
merged 2 commits into from
Jun 24, 2022
Merged

Change ion defaults #32

merged 2 commits into from
Jun 24, 2022

Conversation

dwhswenson
Copy link
Member

If both ions are None and neutralize=True (as is the default) behavior is unclear if the system needs to be neutralized. This changes the defaults to "Na+" and "Cl-".

Second commit here disallows None as a valid value. I don't think there's any reason to allow it if it isn't the default, right?

While I'm at it, should I also change default on ion_concentration to 0.0 molar, instead of None?

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #32 (b65e972) into main (d76805d) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   95.77%   95.66%   -0.12%     
==========================================
  Files          25       25              
  Lines        1112     1106       -6     
==========================================
- Hits         1065     1058       -7     
- Misses         47       48       +1     
Impacted Files Coverage Δ
gufe/solventcomponent.py 95.31% <100.00%> (-1.66%) ⬇️
gufe/tests/test_solvents.py 100.00% <100.00%> (ø)

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 d76805d...b65e972. Read the comment docs.

@richardjgowers
Copy link
Contributor

WRT concentration, there is an argument that None means not-specified whereas 0.0 means actually 0. Should be harmless to set the default to 0.0 though. Something I think you've brought up before that I'm not sure on is, does that concentration include the ions required to neutralise? Because with 0.0 arguably you can't neutralize if it does include those ions. @IAlibay

@dwhswenson
Copy link
Member Author

WRT concentration, there is an argument that None means not-specified whereas 0.0 means actually 0. Should be harmless to set the default to 0.0 though. Something I think you've brought up before that I'm not sure on is, does that concentration include the ions required to neutralise? Because with 0.0 arguably you can't neutralize if it does include those ions.

I believe we're currently just wrapping openmm.app.Modeller, which is explicit in its docs on this:

ionicStrength (concentration=0*molar) – the total concentration of ions (both positive and negative) to add. This does not include ions that are added to neutralize the system. Note that only monovalent ions are currently supported.

@IAlibay
Copy link
Member

IAlibay commented Jun 22, 2022

So in practice neutralize=True with 0 mM ionic concentration just means add the relevant ions to make a neutral system. That's pretty much the standard for it. When you increase the ionic concentration then you essentially adjust the number of ion pairs to aim for that ionic concentration. How that happens in practice is a bit messy, you can look at this code to get an idea of how different methods behave: https://github.com/IAlibay/saltpy/blob/main/saltpy/estimators.py

Copy link
Member

@dotsdl dotsdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @dwhswenson! Merging!

@dotsdl dotsdl merged commit 3ee39f6 into main Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants