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

Compare Ewald with NIST reference #75

Merged
merged 7 commits into from
Feb 9, 2017
Merged

Compare Ewald with NIST reference #75

merged 7 commits into from
Feb 9, 2017

Conversation

Luthaf
Copy link
Member

@Luthaf Luthaf commented Jan 5, 2017

And fix a bug along the way \o/

We do not reproduce very well the 4th NIST configuration results, I'll check if this is improved by changing the Ewald parameters (alpha & k_max) to use the same values as the NIST reference.

@g-bauer
Copy link
Contributor

g-bauer commented Jan 5, 2017

Awesome! I am currently running tests with SPC/E water + Ewald for NVT and NVE to compare to the NIST phase equilibria data and I recognized that pressure is doing weird things, your find might explain why.

I was wondering, what is the reasoning behind

alpha: 3.0 * PI / (cutoff * 4.0)

I used the value given by NIST

α = 5.6 / min(Lx,Ly,Lz)

@Luthaf
Copy link
Member Author

Luthaf commented Jan 6, 2017

alpha: 3.0 * PI / (cutoff * 4.0)

I am afraid I do not remember where this come from. I would like to improve this by allowing the user to only request a precision (say 1e-5), and then compute alpha and kmax to get this precision and enforce the O(n^3/2) behaviour of Ewald.

@g-bauer
Copy link
Contributor

g-bauer commented Jan 8, 2017

Maybe use kmax and tolerance as inputs with alpha set just like in the NIST reference.

@g-bauer
Copy link
Contributor

g-bauer commented Jan 8, 2017

Can we get the test to pass with the values from NIST?

I would like to improve this by allowing the user to only request a precision (say 1e-5), and then compute alpha and kmax to get this precision and enforce the O(n^3/2) behaviour of Ewald.

Is that something you want to add in this PR?

@Luthaf
Copy link
Member Author

Luthaf commented Jan 8, 2017

Can we get the test to pass with the values from NIST?

No, I broke the energy conservation test with these changes, I need to fix it first.

Is that something you want to add in this PR?

No. I'll add it later, in a separated PR =)

@g-bauer g-bauer mentioned this pull request Jan 13, 2017
@Luthaf
Copy link
Member Author

Luthaf commented Jan 18, 2017

I finally found the time to finish this. I added more tests, the Ewald code was broken everywhere.

Now it should be better, but please review carefully the tests, as they where not testing what I though before.

@Luthaf Luthaf force-pushed the tests-nist-ewald branch 2 times, most recently from a64a32c to 29ec758 Compare January 22, 2017 22:14
@g-bauer
Copy link
Contributor

g-bauer commented Jan 23, 2017

Still working through your changes. I didn't implement Ewald summation myself yet so I am very slow.
I will very naively put some questions in here, where I don't understand the equations. I hope that's ok.

You mention this in the tests in the comments. Could the large deviation in the 4th sample come from the different alpha values?

  • NIST: alpha = 5.6 / 30.0 = 0.1867 (4th example)
  • Lumol: alpha = 3*pi / (9.0 * 4.0) = 0.2618 (4th example, cutoff = 9 A)
    The difference is smaller for the smaller boxes of examples 1-3.

@Luthaf
Copy link
Member Author

Luthaf commented Jan 24, 2017

I hope that's ok.

That's totally ok!

Could the large deviation in the 4th sample come from the different alpha values?

Yes, it does. In the last commit, I added a method to set the value of alpha, and used that for nist-spce tests. This make the relative difference in energy two order of magnitude smaller, at the cost of making the 9A/nist-3 test a bit worst (from 1e-5 to 1e-3).

@Luthaf
Copy link
Member Author

Luthaf commented Feb 3, 2017

Ping @g-bauer. Again, if you do not have time, I can do a review of this myself. It is old enough that I forgot about how I've done it 😃.

@g-bauer
Copy link
Contributor

g-bauer commented Feb 3, 2017

Sorry for the delay. I haven't had the time to go through it in one iteration. I think it is better if you do the review. I will continue to go through it, but I don't know when I'll have the time.

The tests passing are a good sign that the implementation is correct. We could further check the different contributions separately, like in the NIST benchmark, section 6).

@Luthaf
Copy link
Member Author

Luthaf commented Feb 9, 2017

I re-read this and rebased it on top of master. I am waiting for Travis to go green, and then I'll merge the PR.

We could further check the different contributions separately, like in the NIST benchmark, section 6).

We can not do this, because the corresponding functions are private. We could make them public, but this seems very ad-hoc, and I do not see how users of the code would need these functions.

@g-bauer
Copy link
Contributor

g-bauer commented Feb 9, 2017

That's fine. It was just an idea to further validate the code but if it passes the tests it should be fine.

I am waiting for Travis to go green, and then I'll merge the PR.

🎉

@Luthaf Luthaf merged commit e8598e1 into master Feb 9, 2017
@Luthaf Luthaf deleted the tests-nist-ewald branch February 9, 2017 21:17
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.

None yet

2 participants