Fix CSVR temperature ramp

[k-harris27]

Fixes #413

Not sure how best to handle a temperature ramp for something without a thermostat, like NVE. Currently it looks like the code just runs the "temperature ramp" anyway. I've put a warning in for now.

Todo:

• Add tests for temperature ramp in each ensemble.

[ElliottKasoar]

Fixes #413

Not sure how best to handle a temperature ramp for something without a thermostat, like NVE. Currently it looks like the code just runs the "temperature ramp" anyway. I've put a warning in for now.

Good question. Is it actually something people would actually do e.g. by setting the energy instead?

If it is nonsensical, or just generally of no use, we could just raise a ValueError if it looks like the user is trying to heat, but has chosen the wrong ensemble.

Is it actually changing anything in the case where it raises a warning? If not, it's probably better to stop it, rather than run a long simulation doing nothing meaningful.

Edit: Ah, it still sets the correct velocity distribution. I think it goes back to the above about how useful/meaningful it is. If it makes sense, I don't think we necessarily need a warning, and if it doesn't, perhaps we shouldn't allow it?

[ElliottKasoar]

The changes for NVT_CSVR look sensible, thanks!

I think it would be too long to go into a unit test, but it's worth confirming manually that this gives a reasonable temperature ramp.

[k-harris27]

I think it would be too long to go into a unit test, but it's worth confirming manually that this gives a reasonable temperature ramp.

This is only short, but I think shows a pretty confident ramp. I can do a longer one on the Manchester HPC if needed.

from ase.build import bulk
from janus_core.calculations.md import NVT_CSVR

NaCl = bulk("NaCl", "rocksalt", a=5.63, cubic=True)
NaCl = NaCl * (3,3,3)

heating = NVT_CSVR(
    struct = NaCl,
    arch="mace_mp",
    device="cpu",
    model_path="small",
    steps=1000,
    temp_start=20.0,
    temp_end=300.0,
    temp_step=20,
    temp_time=100,
    stats_every=10,
    taut=10.
    )

heating.run()

[alinelena]

Fixes #413

Not sure how best to handle a temperature ramp for something without a thermostat, like NVE. Currently it looks like the code just runs the "temperature ramp" anyway. I've put a warning in for now.

Todo:

• Add tests for temperature ramp in each ensemble.

for nve simple stop with an error makes no sense.. and warnings can be easily overseen

[ElliottKasoar]

Looks great so far, thanks!

[k-harris27]

Right now, a test for NVT-NH fails due to #424. This will be fixed when #429 is merged.

[ElliottKasoar]

Generally looks great, just a couple of minor suggestions for the tests

[alinelena]

just to say... nve is not for heating... or cooling

[ElliottKasoar]

just to say... nve is not for heating... or cooling

That's part of these changes, no?

if self.ramp_temp and self.ensemble in ("nve", "nph"):
    raise ValueError(
        "Temperature ramp requested for ensemble with no thermostat."
    )

(Previously, we just didn't set the temperature)