included celllismap example

[lmiq]

Followup of: https://github.com/JuliaDynamics/Agents.jl/issues/659

I included here the files for the CellListMap.jl integration example. I tried to follow the style of the other examples (but included a SVG figure in the examples dir, not sure if that is what you would like - I linked the final raw image in the markdown).

The video that should be produced is similar to this one: https://github.com/lmiq/agents_with_celllistmap/blob/main/celllistmap.mp4

[Datseris]

@lmiq thanks, I am working on this now. Question. Why are forces in the Properties struct, and positions in the CellListMapData struct? What decides which goes where?

[Datseris]

I think there is no reason for Properties to exist. forces, cutoff are also "required data" for CellListMap.jl so it doesn't make sense to separate them.

[codecov-commenter]

Codecov Report

Merging #669 (8cefe49) into main (8312904) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #669   +/-   ##
=======================================
  Coverage   90.42%   90.42%           
=======================================
  Files          27       27           
  Lines        1733     1733           
=======================================
  Hits         1567     1567           
  Misses        166      166           

Impacted Files	Coverage Δ
src/spaces/continuous.jl	92.89% <ø> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Datseris]

@lmiq I removed the .svg image. We try to not include images or any large files in the Agents.jl repo as they accumulate over time. Especially in this case I do not think this image added much useful information to the integration example. It's a plot of a function someone can quite easily create themselves if need be.

[Datseris]

@lmiq from a design point of view, why would the user have to care about updating these threaded data structures...? Can't you create a central structure that would contain things like

mutable struct CellListMapData{B,C,A,O}
    positions::Vector{SVector{2,Float64}}
    forces::Vector{SVector{2,Float64}}
    cutoff::Float64
    box::B
    cell_list::C
    aux::A
    output_threaded::O
end

and provide functions like reset_forces!(forces, datastructure) that does the zero part automatically, and in general handles things automatically?

Perhaps it is too much glue code for the user to have to carry around things like the Box. The user never actually cares to use the box. Just the positions and forces. Wouldn't it make sense to wrap all these stuff into a struct and provide functions that operate on this struct, and take in positions and forces vectors and update them...? My understanding, at least with this example, that the output the user cares about is forces and the input the user cares to give is positions, forces. So, all other things could be wrapped in a structure and passed around along with positions, forces.

To make the point more clear, at the moment I see in the code

    CellListMap.map_pairwise!(
        (x, y, i, j, d2, forces) -> calc_forces!(x, y, i, j, d2, forces, model),
        model.forces,
        model.clmap.box,
        model.clmap.cell_list;
        output_threaded=model.clmap.output_threaded,
        parallel=model.parallel
    )

seems to me that all arguemnts after forces could be in one internal data structure. The user never actually uses the box, the cell_list or aything else. The user only carries them around to pass them to map_pairwise!.

[lmiq]

Yes, in this example I can do that (in general I'm not sure, there are some customized problems that may require the current interface).

I will do that here and maybe add that as a possibility in the package.

[lmiq]

Why are forces in the Properties struct, and positions in the CellListMapData struct?

What I tried is to separate what would be necessary for the use of CellListMap from what is required in an implementation that would not use it. As far as I understand, even without CellListMap, it would be interesting to have a forces vector to contain the forces acting on each particle, to be updated at each iteration. The positions vector, on the other side, is redundant, because the same information is contained in agent.pos fields, but CellListMap needs that additional array in order to not allocate a new instance of it at every iteration.

and provide functions like reset_forces!(forces, datastructure)

I agree with this one in this example, particularly from the point of view of the above. With that the CellListMap "part" would be more self-contained still. I can do that.

I think there is no reason for Properties to exist. forces, cutoff are also "required data" for CellListMap.jl

This is more "controversial". I think that what is needed for CellListMap should be separated from what is needed to run the simulation in general. The cutoff has one additional detail: It could be used as an input parameter in the definition of the space, to set the the finer subspace possible to tune the performance of the interacting_pairs you have implemented. We are not using it now, but it could be used for that (although I'm not sure how much that can help).

[lmiq]

Wouldn't it make sense to wrap all these stuff into a struct and provide functions that operate on this struct, and take in positions and forces vectors and update them...?

I started to do that... but here what we actually get is that model_step! is identical to what an update_forces! function would look like. We need, inside the function, the model data structure, because the parameters of the particles (radii and force constants) are properties that are carried on the agents.

Thus, although the code looks somewhat more organized, there is no real gain in conciseness, unless I put some of that into a new interface of CellListMap (which is not out of question and won't be breaking, so I can certainly see if I arrive to something simpler in these lines).

I will push the changes, but I'm not sure if they are in the right direction.

[lmiq]

@Datseris:

I have significantly changed the example, after creating a new interface for the use of CellListMap which will simplify its, particularly in these examples.

The branch containing the new interface is: https://github.com/m3g/CellListMap.jl/tree/PeriodicSystem

I have still to add some functionality and test the new interface, but in general that is what I arrived at.

I would like to know if you have other suggestions for this interface (concerning specifically how the example looks like now here), before I actually merge all that into CellListMap.

For the moment, I could do that without introducing breaking changes, so that's fine.

[Datseris]

@lmiq please ping me when you think I should give another review and merge

[lmiq]

@lmiq please ping me when you think I should give another review and merge

Sure, I'll tell you when I release the corresponding version of CellListMap.

[lmiq]

@Datseris I think its done now.

[lmiq]

As a side note, I've seen now that you have a "elastic collisions" function implemented. It would be nice to have an example of that as the function to mapped among pairs. I may try to add that example as an integration example on my side, than we can link the examples.

[Datseris]

Notice that the docs are erroring: https://github.com/JuliaDynamics/Agents.jl/runs/7838758532?check_suite_focus=true#step:8:160

don't forget to put the package into the docs/Project.toml file!

[lmiq]

Notice that the docs are erroring: https://github.com/JuliaDynamics/Agents.jl/runs/7838758532?check_suite_focus=true#step:8:160

don't forget to put the package into the docs/Project.toml file!

Added the new doc dependencies. I'm not sure if the error in the docs is related to this pull request, it seemed to be a documenter key access problem.

[Datseris]

Thank you very much @lmiq ! I merged this in, because if we want to alter the "elastic collision stuff", it is probably better to do in a different PR anyways. Or you can do this on your end and then I can link your docpage here!