adapt code to the new interface of CellListMap

[lmiq]

Adapt the code for the interface of CellListMap 0.10.

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.97%. Comparing base (154bd7e) to head (45e3262).

Files with missing lines	Patch %	Lines
src/neighbors.jl	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   65.93%   65.97%   +0.04%     
==========================================
  Files          56       56              
  Lines       13150    13123      -27     
==========================================
- Hits         8670     8658      -12     
+ Misses       4480     4465      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lmiq]

Hi @jgreener64

I think this is ready for review.

One thing that called my attention, though, is that to create a TriclinicBoundary neighbor finder with CellListMapNeighborFinder we need to call the constructor with a TriclinicBoundary object as the unit cell, otherwise it will create a orthorhombic boundary, and that cannot be updated (changed to a different boundary type) later. If CellListMapNeighborFinder is only called from within the System constructor that is internal and is fine, but if not that might create some interface inconsistencies.

Concerning the unit_cell = nothing case: Is this supposed to mean that the system is not periodic? If that is the case, we can remove some code there because CellListMap deals with that internally (and actually through a faster path now). We would not need to create a fake cubic box with twice_cutoff for that.

An unrelated comment:

I think having this method:

Molly.jl/src/types.jl

Line 617 in 154bd7e

function Base.append!(nl::NeighborList, list)

in the code base is dangerous, because it allows one to append a list of neighbors that was allocated internally in another NeighborList object beyond the actual effective number of active pairs. If that method is only called from within the append!(nl::NeighborList, nl_app::NeighborList) method, I would remove it and define this one directly as:

function Base.append!(nl:NeighborList, nl_app::NeighborList)
    for i in eachindex(nl_app)
        push!(nl, nl_app.list[i])
    end
    return nl
end

which avoids that possible misuse. (the use of eachindex here is just because we don't have iterate yet for this type, which could be something sensible to have as well).

[jgreener64]

Looks good, thanks for this. Seems like there is a test failure in test/constraints.jl.

Perhaps the ambiguities around unit_cell, TriclinicBoundary and nothing can be resolved by requiring unit_cell to be provided as an AbstractBoundary? Then it is assumed that the cell type won't change in future, though the dimensions can. n_infinite_dims could be used to check fully infinite boundaries and use a different code path if this helps.

in the code base is dangerous, because it allows one to append a list of neighbors that was allocated internally in another NeighborList object beyond the actual effective number of active pairs

We use this method more generally to append to neighbour lists, so I think we need it. Let me know if there are cases in the code currently where it is misused.

[lmiq]

The issue with the boundary type is that the CellListMap.ParticleSystem object is specialized for the type of unit cell (non-periodic, orthorhombic, or triclinic), with different code paths for each, performance decreasing in that order. This, it is nice to have the unit cell definition at the moment of it's construction. If not one can set it to triclinic and use the slower path in all cases. I'll expand and explore the possibilities asap.

Concerning the append!, I didn't find any bug, it was me that did that and took me some time to realize what was going on. It is just a suggestion.

[lmiq]

@jgreener64

I'm working this still, and did some updates to CellListMap to simplify the interface here.

Nevertheless, there's one thing which CellListMap deals differently, which is the fact that the definition of the unitcell is required for the setup of the CellListMap.ParticleSystem structure.

We have three options:

1. Have the CellListMapNeighborFinder object carry the boundary type. That is what I think is more reasonable, but it breaks the symmetry with the other constructors, and we need to branch the tests for each neighbor finder type.

2. Have the CellListMapNeighborFinder object to always define a Triclinic system internally, which can handle the other types of boundaries, but less efficiently. This requires some workarounds to define the cell size, as was done before.

3. Carry within the CellListMapNeighborFinder object all three alternatives, allowing the user to switch between types of boundaries in the same Molly.System, and efficiently at least if the boundary does not change all the time.

From what I'm used, Molly.System would never change the boundary type within a simulation, right? In that case, having the neighbor finder constructors as part of the interface has any utility? If not, I imagine that making all these constructors completely internal would simplify the interface. One would pass to Molly.System the type of the neighbor finder instead of the instance of it, and let the System constructor call with the appropriate parameters the correct finder, with, possibly the information of the boundary type already given.

[jgreener64]

I favour option 1, best to be explicit on the Molly side. The next release will be breaking (1.0) so now is a good time.

From what I'm used, Molly.System would never change the boundary type within a simulation, right?

That's right.

In that case, having the neighbor finder constructors as part of the interface has any utility?

One advantage is that it allows different neighbour finders to have different setup arguments. That path to setup a System can be fiddly but allows maximum flexibility. The setup path from file in src/setup.jl is more automated and uses neighbor_finder_type as you suggest. Most users of biomolecular simulation would use that path.

[lmiq]

HI @jgreener64

Second attempt. :-)

The only thing that is perhaps worth bringing to your particular attention is that build_neighbor_finder has an additional boundary keyword that has to be passed over to CellListMapNeighborFinder. But it does not seem to be public.

Otherwise the interface changes are only those of the CellListMap interface.

[jgreener64]

Great, thanks for this.