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

Improve LB interface testing #4049

Merged
merged 13 commits into from
Dec 20, 2020
Merged

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Dec 16, 2020

Description of changes:

  • rename print_vtk_* functions to write_vtk_* functions in LB and EK to better reflect what they actually do
  • simplify VTK bounding box code and make it coherent with the numpy slice syntax (i.e. bb2 now needs zero-based indices)
  • add assertions to catch an out of bounds bug in VTK (incorrect bounding box values caused the python interpreter to freeze)
  • fix broken LB getters in the core and python interface
  • fix incorrect LB density getter/setter check in the testsuite
  • add extra checks for LB parameters
  • add testcase for VTK output

Also thermalize the fluid and document the non-VTK write functions.
Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

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

thanks for those tests! just minor improvements should be done before merging

src/core/grid_based_algorithms/lb_interface.cpp Outdated Show resolved Hide resolved
src/core/grid_based_algorithms/lb_interface.cpp Outdated Show resolved Hide resolved
src/python/espressomd/lb.pyx Show resolved Hide resolved
testsuite/python/lb_vtk.py Outdated Show resolved Hide resolved
This change better reflects what is actually carried out by these
functions. Affects LB and EK modules.
Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

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

LGTM, please clarify the private id function which must have been introduced elsewhere

@@ -30,7 +30,7 @@
import espressomd.io.writer
try:
import h5py # h5py has to be imported *after* espressomd (MPI)
skipIfMissingPythonPackage = ut.case._id
skipIfMissingPythonPackage = utx._id
Copy link
Member

Choose a reason for hiding this comment

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

okay, i still don't get why this should be a private function

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed it to no_skip in 08a674f

try:
import MDAnalysis as mda
from espressomd import MDA_ESP
skipIfMissingPythonPackage = ut.case._id
skipIfMissingPythonPackage = utx._id
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skipIfMissingPythonPackage = utx._id
skipIfMissingPythonPackage = lambda x: x

Copy link
Member

Choose a reason for hiding this comment

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

this would be even more readable, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

we want to hide implementation details of python decorators (that's why unittest created _id() in the first place)

also, that lambda expression would be transformed into a regular function by autopep8

@jngrad jngrad force-pushed the lb_interface_coverage branch from 7db0322 to 89586a0 Compare December 18, 2020 09:36
Copy link
Contributor

@christophlohrmann christophlohrmann left a comment

Choose a reason for hiding this comment

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

Changes after Kai's review look good to me

@jngrad jngrad requested a review from reinaual December 18, 2020 14:32
@jngrad jngrad added the automerge Merge with kodiak label Dec 20, 2020
@jngrad jngrad dismissed KaiSzuttor’s stale review December 20, 2020 13:07

requested changes have been addressed

@kodiakhq kodiakhq bot merged commit 0968470 into espressomd:python Dec 20, 2020
@jngrad jngrad deleted the lb_interface_coverage branch December 20, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants