Add :diagonal option to test_approx_equal

[ejmeitz]

Added option :diagonal to AtomsBaseTesting test_approx_equal. The current options are not compatible with Molly.jl (triclinic & DirichletZero) and I would like to test against a valid system.

Also is DirhichletZero supposed to be a hard wall or infinite domain or something else? Doesn't really make a difference for Molly but its not really clear from docs and not something I've ever seen in a molecular simulation context.

[mfherbst]

Lgtm.

Could you also use it one of the tests to ensure it's covered?

The dirichletzero is meant to be a zero bc at the end of the simulation cell. Makes sense for isolated systems. Potentially the cell is infinite, but might not be (e.g. finite element cases).

[ejmeitz]

In that case, DirichletZero is not supported by Molly at all. For now I could add an option to make the test system purely periodic, but I'm also a little confused as to why Infinite is not just another boundary option? I see there is some support but its in a different part of the interface.

[mfherbst]

In that case, DirichletZero is not supported by Molly at all.

What you can always do is warn the user about this and simply ignore the boundary condition (BC). Thus it will effectively always be periodic. The other option is to error out. I don't think it is a good idea to add an option to Molly to change the BC. Rather we should have a generic way in AtomsBase to change BCs.

I'm also a little confused as to why Infinite is not just another boundary option

Because the size of the simulation box and the BC are two different concepts. You can have periodic, dirichlet, neumann, ... BCs also for infinite cells.

[ejmeitz]

Tests should pass now.

Sounds good, that shouldn't be too crazy to add right? I can make a different PR, but just to outline what I'm thinking:

• A function update_boundary_condition(sys::AbstractSystem, new_bc::Vector{Union{Periodic,DirichletZero}) and it just copies over the data from sys into a new AbstractSystem with the bc passed.
• Might also be nice to support only semi-infinite domains (e.g. Inf in only a few dimensions)

[mfherbst]

I can make a different PR, but just to outline what I'm thinking:

Yes please do. I would not use a different function for this. We have "update constructors". Take a look at the atom implementation for example. We should do the same with systems.

[ejmeitz]

@mfherbst Turns out that constructor exists already, is there something else you were referring to than the code below?

https://github.com/JuliaMolSim/AtomsBase.jl/blob/master/src/flexible_system.jl#L87

[mfherbst]

Indeed. Perfect. It seems all that's missing is a nice example in the documentation.

@ejmeitz Would you mind adding one plus a bit of explanation?

[ejmeitz]

Already there in the tutorial guess I'm just blind.