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

mdlc (dipolar layer correction) test needs to be checked #1569

Closed
RudolfWeeber opened this issue Oct 26, 2017 · 2 comments · Fixed by #4061
Closed

mdlc (dipolar layer correction) test needs to be checked #1569

RudolfWeeber opened this issue Oct 26, 2017 · 2 comments · Fixed by #4061
Assignees

Comments

@RudolfWeeber
Copy link
Contributor

The fact that the test did not fail in spite of the bug removed in #1568 suggests that the test didn't test the method sufficiently. The test data uses sqrt(3) as dipole moment, so the unintended cast to int in the calculation of the total dipole moment should have made a difference.

@RudolfWeeber RudolfWeeber reopened this Feb 6, 2018
@jngrad jngrad added the Bug label Dec 7, 2020
@jngrad
Copy link
Member

jngrad commented Dec 7, 2020

Also the test (dipolar_mdlc_p3m_scafacos_p2nfft.py) could be modernized:

  • remove print statements conveying the same information as one of the assertions
  • use float(np.genfromtxt()) instead of python2-style file open
  • deduce n_particle from the size of the particle array loaded from file
  • instantiate all particles at once with numpy slicing
  • maybe replace the duplicated code logic for err_* variables by a class static method to calculate the standard error or the mean from two input variables (the sample size can be deduced from those inputs, no need to involve data.shape[0])

@jngrad
Copy link
Member

jngrad commented Dec 10, 2020

The proposed improvements to dipolar_mdlc_p3m_scafacos_p2nfft.py have been applied in 5ddc3f0.

@kodiakhq kodiakhq bot closed this as completed in #4061 Dec 19, 2020
kodiakhq bot added a commit that referenced this issue Dec 19, 2020
fixes #1569 closes #4046 

This is based on #4046, minus the last commit.

Description of changes:
* Switch tests for dipolar interactions with open boundary conditions from comparing two methods against each other to comparing individual methods against reference data
* Use prefactor != 1 in dipolar 2d and 3d periodic tests
* Block DipolarDirectSumWithReplicaCpu from n_replica=0 and periodic bc, as it does not apply minimum image convention. Introducing minimum image convention there is not worth it, because that is provided by DipolarDirectSumCpu. The two methods should be merged anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants