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

Make Steepest Descent a regular integrator #3891

Merged
merged 22 commits into from
Sep 16, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Sep 10, 2020

Description of changes:

  • Remove the deprecated minimize_energy module
    • use system.integrator.set_steepest_descent() directly
  • Simplify the MPI callback mechanism for Steepest Descent
    • broadcast struct parameters only once during initialization, instead of once per integration
    • call the integrate() MPI callback directly instead of delegating that call to the steepest_descent() MPI callback
  • Document how to set up a custom convergence criterion

jngrad and others added 15 commits August 31, 2020 17:50
The Brownian integrator cannot run without the Brownian thermostat.
This free function was introduced for backwards compatibility with
release 4.1, but it causes too much coupling with the integrator
infrastructure. It can also be confusing to new users.
This parameter was written to the steepest descent struct but never
read from. It was overriden at each call to system.integrator.run().
Broadcast only during initialization instead of once at every call
to system.integrator.run(). Broacast the struct members instead of
the complete struct.
Use the regular callback mechanism, like mpi_integrate(), and call
the main integration callback integrate() directly.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@jngrad
Copy link
Member Author

jngrad commented Sep 10, 2020

This PR depends on #3880 and currently includes it. Here is the relevant diff subset.
@christophlohrmann could you please have a look at the samples and tutorials?

samples/chamber_game.py
samples/drude_bmimpf6.py
samples/visualization_bonded.py
samples/visualization_cellsystem.py
samples/visualization_charged.py
samples/visualization_elc.py
samples/visualization_npt.py
doc/tutorials/02-charged_system/02-charged_system-1.ipynb
doc/tutorials/02-charged_system/02-charged_system-2.ipynb
doc/tutorials/02-charged_system/scripts/nacl.py
doc/tutorials/02-charged_system/scripts/nacl_units_confined.py
doc/tutorials/02-charged_system/scripts/nacl_units_confined_vis.py
doc/tutorials/04-lattice_boltzmann/04-lattice_boltzmann_part3.ipynb
doc/tutorials/04-lattice_boltzmann/scripts/04-lattice_boltzmann_part3_solution.py

@christophlohrmann
Copy link
Contributor

From manual check: Visualizers look good, systems do not explode

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 except for the long testcase that needs to be shortened somehow.

src/core/integrators/steepest_descent.cpp Show resolved Hide resolved
testsuite/python/integrator_exceptions.py Outdated Show resolved Hide resolved
@jngrad
Copy link
Member Author

jngrad commented Sep 15, 2020

There is a silent regression somewhere. I get random failures in the ROCm image for tests dawaanr-and-dds-gpu and dawaanr-and-dds-gpu that look like UB (logfiles 1, 2, 3), but I can't reproduce them on lama nor in the ROCm docker image on lama. They are only reproducible in the docker image on coyote10, but not every time, and in GDB the backtrace is sometimes squashed. The few times I got a usable backtrace in GDB, it was about incorrect particle ids and double free of the cellsystem particle list (dawaanr-and-dds-gpu traces 1.log, 2.log).

The regression is independent of 022ee4e and independent of another silent regression that was fixed by 112bd65.

@jngrad
Copy link
Member Author

jngrad commented Sep 15, 2020

Even more failures after the bugfix in 112bd65 (logfile):

	 32 - dawaanr-and-dds-gpu (Failed)
	 33 - dawaanr-and-bh-gpu (Failed)
	 34 - dds-and-bh-gpu (Failed)

This is not a regression, the bug is already present on the python branch. Tested on fe967ea with the ROCm image on coyote11: dawaanr-and-dds-gpu, dawaanr-and-bh-gp and dds-and-bh-gpu all fail randomly. It's also failing in other PRs.

doc/sphinx/running.rst Outdated Show resolved Hide resolved
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.

after my comments have been addressed, this can be merged

@KaiSzuttor KaiSzuttor added the automerge Merge with kodiak label Sep 16, 2020
@kodiakhq kodiakhq bot merged commit f732f79 into espressomd:python Sep 16, 2020
@jngrad jngrad deleted the remove-minimize branch January 18, 2022 12:16
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