lumol-org / lumol

Universal extensible molecular simulation engine
http://lumol.org/
BSD 3-Clause "New" or "Revised" License
184 stars 18 forks source link

Enable potential cutoff radius that is larger than half the cell size in a periodic system #232

Closed droundy closed 4 years ago

droundy commented 6 years ago

Looking at the implementation of pairs:

http://lumol.org/lumol/latest/src/lumol_core/sys/energy.rs.html#43-54

It seems that the code is incorrect unless the cell size (for a periodic cell) is at least twice the cutoff radius of the potential. This seems like an unfortunate limitation.

Luthaf commented 6 years ago

Yes, the code uses minimal image convention by default.

I agree that this is a limitation, but I am wondering how often this is an issue in practise. With a cutoff value of 8 A, one need a cell of at least 16 A to be OK, and this already looks like a very small cell to me. I typically use at least 20-30 A for the cell in my simulations. Is your experience different ?

If we want to go all the way to the cutoff, even if it is bigger than what is contained in the unit cell, what is the algorithm to do it? Do we need to loop over all the cell surrounding the current one, until the distance is bigger than the cutoff, or is it any clever algorithm?

We emit a warning in Monte Carlo simulation is the cell become smaller than twice the cutoff, and we should do the same for molecular dynamics, to make sure the user know what is going on.

droundy commented 6 years ago

My concern would be that testing for cutoff radius convergence would fail, as the cutoff exceeds half the cell size. I could also imagine trouble if lumol is ever used for solids.

Yes, looping over cells would be the solution.

I'd rather see a crash than a warning when the configuration would result in wrong answers. It is very easy to miss warnings.

Luthaf commented 6 years ago

I'd rather see a crash than a warning when the configuration would result in wrong answers. It is very easy to miss warnings.

It is actually a crash (fatal_error! expands to panic!), my bad.

I am not opposed to implement looping over the cells images, but I am concerned about performances of doing this. What do you think of a systematic crash telling the user to use a bigger system ?

droundy commented 6 years ago

I would check on cell creation whether the cell is big enough and store the result. Then all that is added to the fast path is a single if, which checks whether looping is required. This shouldn't be a significant slow down.

A crash, however would be acceptable.

Luthaf commented 6 years ago

Yeah, branch prediction could help here in the fast path. But we would need to recheck the cell every time it changes (for NPT simulations).

I think a crash is easier to implement and can be a first step on this issue.

Luthaf commented 4 years ago

We currently crash if the cell is not big enough to assume minimal image convention. I think this should be OK in most of the cases: users can duplicate the cells themself if needed.

If you still think we should support cutoffs larger than half the unit cell, please comment here explaining why you cannot use the above strategy.