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

Python maintenance #4516

Merged
merged 9 commits into from
May 31, 2022
Merged

Python maintenance #4516

merged 9 commits into from
May 31, 2022

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented May 25, 2022

Description of changes:

  • user guide:
    • cleanup code examples and fix bond breakage documentation (e.g. mode "bind_centers" vs. "bond_centers")
    • document ghost particles side-effects on observables
    • where changes were made, wrap text to ~80 chars per line to make git diffs more readable
  • python testsuite:
    • reduce code duplication by merging similar tests together and by using tests_common.py more often
    • improve code coverage by rewriting assertions
  • bug fixes:
    • the incorrectly implemented slab search mode of system.analysis.nbhood() was removed (fixes Slab neighborhood search is broken #4517)
    • TabulatedNonBonded.is_active() now returns False instead of None when the bond is inactive
    • always checks for runtime errors when the cell system changes or when a ScriptInterface object is instantiated
  • API changes:
    • script interface methods no longer return True to signal they didn't raise an exception
  • script interface improvements:
    • simplify System class and document why the _Globals class (now renamed to _BoxGeometry) has to exist
    • simplify ScriptInterfaceHelper classes using automatically generated member methods
    • simplify several core functions that are included in pxd files

@jngrad jngrad force-pushed the python_maintenance branch from 806d3cd to a84e824 Compare May 25, 2022 08:38
jngrad added 8 commits May 25, 2022 16:35
Fix code samples, spell check and grammar check, cleanup docstrings.
`TabulatedNonBonded.is_active()` now returns False instead of None
when the bond is inactive. Let ScriptInterface classes automatically
generate interface methods. Always checks for runtime errors when
the cell system changes or when a ScriptInterface object is created.
Don't return True from methods that can return void (relic from the
ES_OK / ES_ERROR signaling strategy that has been replaced by Python
exceptions).
The algorithm searched for particles in a cylinder perpendicular
to the plane instead of searching for particles in a slab parallel
to the plane.
Re-use existing utility functions in tests_common to reduce code
duplication. Replace unreachable exceptions by unittest assertions.
Fix tolerance of LBGPU test case up to single-precision accuracy.
@jngrad jngrad force-pushed the python_maintenance branch from a84e824 to 6d91ec5 Compare May 25, 2022 14:51
@jngrad jngrad added this to the Espresso 4.2 milestone May 25, 2022
@jngrad jngrad marked this pull request as ready for review May 25, 2022 15:30
@jngrad jngrad requested a review from pkreissl May 25, 2022 15:30
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
doc/sphinx/analysis.rst Outdated Show resolved Hide resolved
doc/sphinx/lb.rst Outdated Show resolved Hide resolved
doc/sphinx/lb.rst Outdated Show resolved Hide resolved
doc/sphinx/lb.rst Outdated Show resolved Hide resolved
src/python/espressomd/system.pyx Outdated Show resolved Hide resolved
src/python/espressomd/system.pyx Outdated Show resolved Hide resolved
src/python/espressomd/system.pyx Outdated Show resolved Hide resolved
@@ -235,6 +234,13 @@ cdef Vector3d make_Vector3d(a):
return v


cdef Vector3i make_Vector3i(a):
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be an assert that len(a) == 3?

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 don't think it's necessary, the helper functions make_Vector3i() and make_Vector3d() should only be called on a variable a after it passed the sanity check check_type_or_throw_except(a, 3, float, "...").

src/python/espressomd/visualization.py Outdated Show resolved Hide resolved
Handle 0-dimensional arrays in type checks. Improve error messages.
Cleanup user guide. Convert uppercase comments to lowercase in the
visualizer and remove comments that are redundant with docstrings.

Co-authored-by: Patrick Kreissl <[email protected]>
@jngrad jngrad requested a review from reinaual May 25, 2022 20:27
Copy link
Contributor

@pkreissl pkreissl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the nice code improvements and for putting in the extra work of capitalization in the visualizer!

@@ -92,7 +92,7 @@ class LBGPULinearMomentum(LinearMomentumTest, ut.TestCase):
"""Test for the GPU implementation of the LB."""

lb_class = espressomd.lb.LBFluidGPU
atol = 1e-6
atol = 1e-5
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here that the tolerance needs to be increased?

Copy link
Member Author

Choose a reason for hiding this comment

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

This test is failing at random in CI, e.g. logfile 317102 in the long-range actors refactoring PR. The numerical error is random with a probability distribution that looks Chi-squared. The test fixes all random seeds and the core is supposed to sum the densities in a deterministic order, so I'm not sure what the root cause is. I ran that test in a loop for several thousand times and the largest deviation was 4e-6.

@jngrad jngrad added the automerge Merge with kodiak label May 31, 2022
@kodiakhq kodiakhq bot merged commit ac4c70b into espressomd:python May 31, 2022
@jngrad jngrad deleted the python_maintenance branch May 31, 2022 08:31
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.

Slab neighborhood search is broken
3 participants