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

Add thermostat/integrator compatibility checks #3880

Merged
merged 8 commits into from
Sep 16, 2020

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Aug 31, 2020

Fixes #3850

Description of changes:

  • add missing thermostat combination decorators
  • document policy on allowed thermostat combinations
  • prevent usage of incompatible thermostat/integrator combinations

@@ -709,6 +710,7 @@ cdef class Thermostat:
mpi_bcast_parameter(FIELD_TEMPERATURE)

IF STOKESIAN_DYNAMICS:
@AssertThermostatType(THERMO_SD)
Copy link
Member

Choose a reason for hiding this comment

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

is no combination of SD with any other thermostat allowed?

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Sep 1, 2020 via email

@KaiSzuttor
Copy link
Member

is no combination of SD with any other thermostat allowed?

I think so.

and why?

@mkuron
Copy link
Member

mkuron commented Sep 1, 2020

That's how SD works. It puts Brownian fluctuations on all particles because it's not only a hydrodynamics solver, but also a momentum-conserving thermostat. Just like you wouldn't put a Langevin thermostat on particles coupled to a thermalized LB, you shouldn't combine SD with other thermostats.

@KaiSzuttor
Copy link
Member

yeah, but you can for example combine LB + DPD because both conserve momentum and add fluctuations on the particles

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Sep 1, 2020 via email

@KaiSzuttor
Copy link
Member

well, it should be documented then. we have to tell the user why we do not allow the combination (even if the reason is that we don't know if works).

@biermanncarl
Copy link
Contributor

biermanncarl commented Sep 1, 2020

Thanks JN. This fixes that both thermostats could be enabled, which is not known to correctly work (yet). However, I still can combine the SD integrator and the Langevin thermostat (and possibly other thermostats, too). Should this be allowed?

I think that maybe, the SD integrator should only be allowed to work with the SD thermostat (and the other way round is obvious, because the SD thermostat depends on the SD intergator).

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Sep 1, 2020 via email

@mkuron
Copy link
Member

mkuron commented Sep 1, 2020

I still can combine the SD integrator and the Langevin thermostat

It would be an odd combination, but probably not outright wrong. It's somewhat like combining an unthermalized LB with a Langevin thermostat -- do we currently disallow that?

@jngrad
Copy link
Member Author

jngrad commented Sep 1, 2020

AFAIK the integrator and thermostat classes don't know about each other in the python interface.

@jngrad
Copy link
Member Author

jngrad commented Sep 1, 2020

well, it should be documented then. we have to tell the user why we do not allow the combination (even if the reason is that we don't know if works).

Currently none of the combination restrictions are documented. This is essentially an allowlist of combinations that are known to work together, which gets expanded whenever we have hard data confirming that two thermostats work together, in opposition to a denylist that would prevent specific combinations for which we can justify they produce wrong physics. I'll write something in the docs to explain this policy.

@jngrad jngrad added this to the Espresso 4.1.4 milestone Sep 1, 2020
@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Sep 2, 2020

AFAIK the integrator and thermostat classes don't know about each other in the python interface.

Yes. It needs to be done in the core, e.g., in on_integration_start(). Once that check is in, this PR is ready, imo.

@jngrad jngrad changed the title Add DPD and SD thermostats compatibility checks Add thermostat/integrator compatibility checks Sep 2, 2020
@jngrad jngrad modified the milestones: Espresso 4.1.4, Espresso 4.2 Sep 2, 2020
@jngrad
Copy link
Member Author

jngrad commented Sep 8, 2020

There are more issues:

  • we can initialize thermostats without a seed to crash the script (the error message from the un-initialized boost::optional is displayed as a char pointer)

    import espressomd
    system = espressomd.System(box_l=[1., 1., 1.])
    system.time_step = 0.01
    system.cell_system.skin = 0.4
    system.part.add(pos=(0, 0, 0), ext_force=[1,0,0])
    system.integrator.set_brownian_dynamics()
    system.integrator.run(1)

    output:

    terminate called after throwing an instance of 'char const*'
    
  • we can use a thermostat after it has been "destructed" in the core with turn_off(), leading to divisions by zero:

    import espressomd
    system = espressomd.System(box_l=[1., 1., 1.])
    system.time_step = 0.01
    system.cell_system.skin = 0.4
    system.part.add(pos=(0, 0, 0), ext_force=[1,0,0])
    system.thermostat.set_brownian(kT=1.0, gamma=1.0, seed=42)
    system.thermostat.turn_off()
    system.integrator.set_brownian_dynamics()
    system.integrator.run(1)
    print(system.part[:].pos)

    output:

    [[inf nan nan]]
    

The Brownian integrator cannot run without the Brownian thermostat.
@kodiakhq kodiakhq bot merged commit 1d6c706 into espressomd:python Sep 16, 2020
jngrad added a commit to jngrad/espresso that referenced this pull request Sep 16, 2020
Fixes espressomd#3850

Description of changes:
- add missing thermostat combination decorators
- document policy on allowed thermostat combinations
@jngrad jngrad deleted the fix-3850 branch January 18, 2022 12:15
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.

stokesian dynamics affected by langevin thermostat even after thermostat.set_sd()
5 participants