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 exception handling and fix various regressions #4479

Merged
merged 10 commits into from
Mar 22, 2022

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Mar 18, 2022

Description of changes:

  • improve exception handling in MPI code (partial fix for Robust exception mechanism in core setters #4219)
    • several runtime errors used to be queued for far too long and would only surface during integration
      • runtime errors from non-bonded interactions instantiated with a cutoff too large now immediately raise an exception, from which the user needs to recover by either reducing the cutoff or disabling the interaction
      • runtime errors from virtual sites relative applied to particles too far away now immediately raise an exception, from which the user needs to recover by increasing the minimum global cutoff
    • all automatically-generated script interface methods used to call handle_errors("") with an empty string; now it's called with the method name to help identify which feature threw the exception
    • virtual sites fatal errors are now runtime errors
    • quaternion fatal errors are now runtime errors
    • invalid particle ids fatal errors are now value errors
  • fix bugs
    • virtual sites can no longer relate to themselves
    • particles can no longer be created with a negative id
    • LB node property boundary no longer returns a random integer when LB_BOUNDARIES is not compiled in
  • improve code coverage
    • common exceptions are now properly covered by tests (e.g. error messages from the cell system and integrator)
    • rotation code is now unit tested
    • h5md exceptions are now tested
  • fix regressions in the testsuite
    • checkpoint tests were only running on 1 MPI core
    • the LB VTK had a wall boundary misplaced
    • virtual sites relative test cases can now run in any order
  • fix regression in the OpenGL visualizer
    • activating the LB_draw_boundaries option without any other LB_draw_* option no longer throws an exception
    • rescaling the box size in lj-demo.py now properly rescales the particle coordinates (partial fix for lj-demo.py is broken #4347)

jngrad added 7 commits March 17, 2022 17:34
Check Particle setters/getters on empty myconfig files. Fix
incorrect position of the LB boundary in the VTK test. Make
VirtualSitesRelative tests independent from each other and
faster using less particles (runtime scales with n_part^2).
Fix MAX_NUM_PROC regression in the checkpoint test CMake logic.
Use a temporary directory in I/O tests to automate file cleanup.
Improve testing of importlib_wrapper and reduce code duplication.
When a real particle is not found, queue a runtime error instead
of throwing a fatal error.
Add missing calls to handle_errors() and remove superfluous ones.
When calling a script interface method, call handle_errors() with a
clear error message. Catch null quaternions in the script interface
to avoid triggering a fatal error in boost::qvm. Prevent relative
virtual sites from tracking themselves.
Rename `make_map()` to `make_unordered_map_of_variants()` to better
reflect its role. Remove `get_map()` and instead implement getter
`template <class T> get_value<std::unordered_map<int, T>>(Variant)`.
Make Variant runtime error messages human-readable by substituting
the Variant demangled name by the string "ScriptInterface::Variant".
Reduce code duplication.
When LB_BOUNDARIES is not compiled in, the return type changes and
the function pointer stored in the MpiCallbacks framework accesses
the wrong data type.
jngrad added 3 commits March 18, 2022 22:58
Convert fatal errors into ValueError for invalid particle ids.
Make python code for particle creation more readable.
@jngrad jngrad marked this pull request as ready for review March 18, 2022 23:36
@jngrad jngrad requested a review from reinaual March 18, 2022 23:36
@jngrad
Copy link
Member Author

jngrad commented Mar 18, 2022

This work is needed for the electrostatics/magnetostatics refactoring in 4.2.0.

@@ -25,7 +25,12 @@


class Non_bonded_interactionsTests(ut.TestCase):
system = espressomd.System(box_l=[20.0, 20.0, 20.0])
system = espressomd.System(box_l=[30.0, 30.0, 30.0])
Copy link
Contributor

Choose a reason for hiding this comment

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

does this number even matter in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, with 20 on the x-axis the test fails on 3 MPI ranks because the LJ range of 8.4 is too large. This error message never surfaced before because runtime errors were never checked when setting a interaction. It's only when the integrator runs that it throws, which can be really confusing when the integrator is run indirectly, e.g. via P3M tuning...

@jngrad jngrad added the automerge Merge with kodiak label Mar 22, 2022
@kodiakhq kodiakhq bot merged commit f93741b into espressomd:python Mar 22, 2022
@jngrad jngrad deleted the exceptions branch March 22, 2022 14:29
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.

2 participants