add Force Based Motion Planning (FMP)

[rmcsqrd]

Continuation of #412.

I addressed the comments made in #412 (julia syntax formatting, more concise functions for ac,as,am, and moved plotting to InteractiveDynamics).

As a note, I implemented as a new function rather than as a form of update_vel for reasons mentioned here: #412 (comment)

[Datseris]

Hm, the docs in https://juliadynamics.github.io/Agents.jl/previews/PR456 do not appear, @Libbum do you know why? How do the docs build for #447 but not for this one...?

[AayushSabharwal]

There's a missing entry in the CI tasks. Usually when docs build there's an entry for documenter/deploy to view the built docs at the URL you mentioned

[codecov-io]

Codecov Report

Merging #456 (e7e5aa4) into master (8ae4532) will decrease coverage by 5.23%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
- Coverage   92.97%   87.74%   -5.24%     
==========================================
  Files          17       17              
  Lines        1224     1297      +73     
==========================================
  Hits         1138     1138              
- Misses         86      159      +73     

Impacted Files	Coverage Δ
src/spaces/continuous.jl	65.66% <0.00%> (-29.96%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ae4532...e7e5aa4. Read the comment docs.

[Datseris]

I think it was that Colors was not added in the Project.toml...

@rmcsqrd the example file has a huge amount of code duplication. Can you please eliminate code duplication as much as possible by defining a reusable functions with keywords?

For example:

for i in 1:model.num_agents

    ## compute position around circle
    theta_i = (2*π/model.num_agents)*i
    xi = r*cos(theta_i)+x/2
    yi = r*sin(theta_i)+y/2

    xitau = r*cos(theta_i+π)+x/2
    yitau = r*sin(theta_i+π)+y/2

    ## set agent params
    pos = (xi, yi)
    vel = (0,0)
    tau = (xitau, yitau)  ## goal is on opposite side of circle
    radius = model.FMP_params.d/2
    agent_color = agent_init_color(i, model.num_agents)  ## This is new
    add_agent!(pos, model, vel, tau, agent_color, :A, radius, model.space.extent, [], [])
    add_agent!(tau, model, vel, tau, agent_color, :T, radius, model.space.extent, [], [])
end

this shuold be made a function like add_agents_around_circle!(model, N). Similarly, there is no reason to define new as, ac, am functions every time you call abm_video. Do it once and re-use them.

Until the docs build online, can you please build them locally and make sure that they are nice? You can uncomment every other example in makedocs.jl to save time for building locally (but please do not push this change here). I saw some comments that did not use proper Documenter syntaqx, e.g. adding a header like ## Random Positions which starts with ##. Also please remove all the ## potential shape options here: https://gr-framework.org/julia-gr.html comments.

In the meantime, I will look at the "true source" of this PR, which are the changes in the continuous.jl file.

[Libbum]

the docs in https://juliadynamics.github.io/Agents.jl/previews/PR456 do not appear

Because this PR is not coming from the JuliaDynamics repo, instead from rmcsqrd's fork. So we can't build them unfortunately.

[Datseris]

This documentation string needs improvement. Why don't you state all parameters that this struct takes in? I don't think it is useful to refer the user to some paper, when we can simply state this information here.

[rmcsqrd]

That is fair - I will update it to describe the parameters in the doc string. I will note that some of the parameters (c1, c2) seem to be empirically defined hyperparameters. The paper literally just refers to them as "two positive constant values".

In general my thought was to provide fmp_parameter_init() to remove any user guessing about how to parameterize the FMP algorithm by providing the default values used in the original paper. I want to expose the underlying parameters in case users decide they want to modify them for some reason but wanted to make the algorithm as plug and play as possible. My approach is admittedly a little clunky so if you have a better idea of how to approach this I am interested in hearing it.

[Datseris]

@rmcsqrd in the example file you still say

The FMP algorithm is included as a predefined update_vel function for use

but that is no longer true, right?

[Datseris]

Out of these parameters, how many are mandatory for the FMP algorithm? If there are more than just pos, vel, we could extend the @agent macro here.

[rmcsqrd]

I will use the @agent macro in the example. Realistically we need id, pos, vel, tau, type, and Ni.

color is really just for plotting to make the visualizations look nice. radius is right now used just for plotting and in my example I pull it from model.FMP_params.d. This is probably not a good idea because model.FMP_params.d is what is actually used for the FMP force computations. SSdims is really just used for plotting. Gi is a list of goal objects near an agent that I added in for my thesis that isn't arguably part of the FMP implementation but I do think it is useful so that users don't need to poll interacting_pairs themselves. If you want me to remove that utility I am open to it.

I think the big question I have is how to handle plotting? Without modification, the default agent size when plotted is pretty small which is why I have a plot function like as_f(a) = 1200*1/minimum(a.SSdims)*a.radius. As I understand it, when plotting a simulation, the ac, as, am functions only pass the agent as an argument. My preference would be to call model.space.extent in lieu of agent.SSdims but I am not sure how to make that possible. If you have an idea on how to access model within ac,as,am then we could remove SSdims and potentially radius and possibly color.

[Datseris]

FMP_Parameter_Init is mentioned both here and in the source code but doesn't exist. It has to be replaced with fmp_parameter_init.

[Datseris]

It truly doesn't make sense to have this function without stating what keywords it accepts. @rmcsqrd you might be confused with how documentation strings work in Julia: only what you, the developer, writes in the docstring is what a user sees. A user cannot see the keywords a function takes in unless you explicitly tell them what they are.

[Datseris]

The functions fmp_update_interacting_pairs, fmp_update_vel modify their argument, so they should end with !.

[Datseris]

The way this is stated is missleading. This function is not called every time agent_step! is called. Rather, the user must manually call it every time they use agent_step!, for the algorithm to work.

[Datseris]

@rmcsqrd this needs a bit more polish before we continue. One of the things that happens at the moment is that there are a lot of manual demands: the user must do a lot of specific steps and have a specific form of model_step! and agent_step! for this to work. We can improve on that.

But before trying to improve on that, I'll wait for you to return here and do the easy fixes I have already outlined so far.

[rmcsqrd]

@Datseris Thanks for all the feedback. I'm getting busy with writing my thesis so I need to put this PR on the backburner for a little - just FYI. I'll work on addressing the comments when I have a chance.

[Datseris]

No problem, take your time!

[Datseris]

@rmcsqrd I will be finishing this PR over the course of the next 2 days if you can't make it.

EDIT: Ah, this is unfortunate. I cannot finish this PR myself because of the comment:

This documentation string needs improvement. Why don't you state all parameters that this struct takes in? I don't think it is useful to refer the user to some paper, when we can simply state this information here.

I would prefer not having to read the paper to explain all parameters as I'm also short on time. Thus, when you have the time, please provide an explanation of the parameters.

[rmcsqrd]

@Datseris Sure I will work on addressing all your comments today.

[rmcsqrd]

@Datseris I picked up your comments at https://github.com/rmcsqrd/Agents.jl/tree/FMP_2. I was getting an error when I tried to push to my FMP branch. I tried git pull first but it still isn't working. Do you know what I should do?

Rios-MacBook-Pro-2:Agents.jl riomcmahon$ git push fork FMP
To https://github.com/rmcsqrd/Agents.jl.git
 ! [rejected]        FMP -> FMP (fetch first)
error: failed to push some refs to 'https://github.com/rmcsqrd/Agents.jl.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

[Libbum]

Try git pull --rebase

[Datseris]

@rmcsqrd I'll have a look locally!

[Datseris]

@rmcsqrd Is this what the first example is supposed to look like:

output1.mp4

[rmcsqrd]

Yes, example 1 is as depicted in your attached video. Example 2 is same as example 1 but with colors. Example 3 has a line of agents with a moving obstacle. I'll also need to remove the equalaspect=true kwarg because it looks like that got removed in InteractiveDynamics.jl a few hours ago. The InteractiveDynamics.abm_play documentation for Agents.jl doesn't reflect this FYI.

[Datseris]

I already did it in last commit! Wait I'll push now!

[rmcsqrd]

Sweet, thanks for fixing that and verifying the docs. What are next steps on my end? I think I have addressed all of your comments so far.

One thing that I will note is that at some point I think I broke the FMP implementation as noted in #412 (comment) - agents don't come into a gridlock condition when they do a "circle swap" as depicted in output1.mp4/output2.mp4. I think the implementation mostly works but this is a bug that I need to look into.

[Datseris]

What are next steps on my end? I think I have addressed all of your comments so far.

I will have a look locally on how we can streamline the interface from a user's perspective. Something you can do is trying to come up with a meanigful test that we can put in our unit tests.

[Datseris]

@rmcsqrd I'm trying to understand.... why do the targets and obstacles need to be Agents...?

For me, this functionality you contribute here is quite similar to the Pathfinding functionality that was implemented in PR #447 . There we realized that most things related with pathfinding should not be agents but part of the model or space. Do you think something similar can be done here, or indeed every goal and every obstacle needs to be an agent?

I see that when you create the agents, the fields Ni, Gi are empty. The function fmp_update_interacting_pairs! sets their values. I honestly think we can out all the Ni, Gi information into a new, dedicated struct that is updated instead, just as we do in the Pathfinding implementation. Please do check and let me know if this is true or not. If the user does not access the fields Ni, Gi, then they shouldn't create them themselves at all. In our new struct we can make a dictionary mapping integers (IDs) to the corresponding Ni, Gi values each agent should have. This would also eliminate having targets and obstavles as agents and reduce the complexity the user faces a lot.

(Side remark: Ni::Array{Int64} is improper and bad for performance. Array{Int64} is an abstarct type)

EDIT: Also, I think we should rename all FMP stuff to FBMP, as the algorithm is not Forced Motion Planning (which is what I thought aaaaall this time) but Force Based Motion Planning :D

EDIT 2: out of the fields of Fmp_Agent, which are absolutely mandatory for the FBMP algortithm?

[Datseris]

@rmcsqrd some side comment: color_select != "none" is never done in Julia. It is something standard in Python. In Julia non-existing values are represented by the standard type nothing. You can conveniently write !isnothing(color_select) in your boolean checks then.

[rmcsqrd]

@rmcsqrd some side comment: color_select != "none" is never done in Julia.

Noted, just Python habits coming over. I will modify.

For me, this functionality you contribute here is quite similar to the Pathfinding functionality that was implemented in PR #447 . There we realized that most things related with pathfinding should not be agents but part of the model or space. Do you think something similar can be done here, or indeed every goal and every obstacle needs to be an agent?

"Because that's the way I wrote it" 😁
This is a fair question and I haven't considered the alternative of moving them to model space. I also haven't looked at the A* PR so I'll check out what was one there. My off-the-cuff and potentially naive response is:

1. I think giving users the option to move obstacles/targets is useful functionality and I am unclear if that functionality exists for non-agent entities. The only real distinction between :A and :O,:T types are what forces they experience (this is elaborated on in the top section of examples/FMP.jl). As mentioned, I don't know what was done in the A* implementation but I am interested what the motivation is for not having them as Agents.
2. The tooling for computing distance seems to naturally fit within the interacting_pairs function. Moving obstacles/targets into the model space seems like it might require reinventing that wheel if they are not agents.

I see that when you create the agents, the fields Ni, Gi are empty ....

All this makes sense. When you're talking about Ni Gi structs would this be some global interaction array where N(i,j) == 1 implies that the ith and jth agent are interacting? Or would the struct still be attached to the Agent type implicitly similar to how id is assigned automatically? I could see either working but I think I prefer the latter because polling model[agent_id].Ni\model[agent_id].Gi and having it return a list seems very clean to me. Those two options probably are not mutually exclusive either.

(Side remark: Ni::Array{Int64} is improper and bad for performance. Array{Int64} is an abstarct type)

What is better? I know it is going to be a list of Int64's. (this stuff is a weakpoint in Julia for me)

EDIT: Also, I think we should rename all FMP stuff to FBMP, as the algorithm is not Forced Motion Planning (which is what I thought aaaaall this time) but Force Based Motion Planning :D

FMP is what the authors refer to the algorithm as in the original paper - I'll defer to you on what you want to call it but my preference is to stick with the authors.

EDIT 2: out of the fields of Fmp_Agent, which are absolutely mandatory for the FBMP algortithm?

mutable struct FMP_Agent <: AbstractAgent
id::Int
pos::NTuple{2, Float64}
vel::NTuple{2, Float64}
tau::NTuple{2, Float64}
color::String
type::Symbol
radius::Float64
SSdims::NTuple{2, Float64}
Ni::Array{Int64}
Gi::Vector{Int64}
end

Whether or not they live in the Agent struct or somewhere else is up for debate but those are all required in some way.

[rmcsqrd]

Just to clarify the algorithm is styled as "Force-based Motion Planning" - I mislabeled it in the PR title and probably everywhere else.

[Datseris]

@rmcsqrd We don't have to port everything to be part of the space, but we should try to eliminate all the manual tuning from the user that has to explicitly make things like the model stepping function being exactly like:

function model_step!(model)
    fmp_update_interacting_pairs!(model)
    for agent_id in keys(model.agents)
        fmp_update_vel!(model.agents[agent_id], model)
    end
end

Why don't you wrap this above to a single function fmp_apply_forces!(model) that contains what you write above? Then the users can use this function in their model.

It is not necessary to put everything to be property of the space. although it could lead to a cleaner set up in the end. it is a bit tricky to see at the moment. Let's start streamlining this piece by piece, and the picture of what is the exported api will become clearer. then we can see if we can put things somewhere specific.

I think giving users the option to move obstacles/targets is useful functionality and I am unclear if that functionality exists for non-agent entities. The only real distinction between :A and :O,:T types are what forces they experience (this is elaborated on in the top section of examples/FMP.jl). As mentioned, I don't know what was done in the A* implementation but I am interested what the motivation is for not having them as Agents.

Yes, that is a valid point. And the "types" are also valid. To streamline this, you can provide a custom agent sub-type, e.g. FBMPAgent so that the users can use the @agent macro to make this agent. You only need to define the struct FBMPAgent that contains all absolutely mandatory fields, e.g. how we do it at the open street map:

mutable struct OSMAgent <: AbstractAgent
    id::Int
    pos::Tuple{Int,Int,Float64}
    route::Vector{Int}
    destination::Tuple{Int,Int,Float64}
end

and then the users can inherit from it using the @agent macro.

---

The examples are very convoluted regarding the agent fields and coloring. Perhaps we can simplify this as well? The agents do not need to have a "color" field. A function can decide the color based on the agent ID and or agent "type".

---

Because we need to get out a stable version soon due to a manuscript we have submitted, this might not make it into v4.2. But I promise you it will make it into v4.3!

[rmcsqrd]

@Datseris Thanks for the comments - all of that sounds like a good direction.

Because we need to get out a stable version soon due to a manuscript we have submitted, this might not make it into v4.2. But I promise you it will make it into v4.3!

That totally works for me. I am graduating in May so things are getting pretty busy for me right now.

[codecov-commenter]

Codecov Report

Merging #456 (e7e5aa4) into master (8ae4532) will decrease coverage by 5.23%.
The diff coverage is 0.00%.

❗ Current head e7e5aa4 differs from pull request most recent head cee2f34. Consider uploading reports for the commit cee2f34 to get more accurate results

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
- Coverage   92.97%   87.74%   -5.24%     
==========================================
  Files          17       17              
  Lines        1224     1297      +73     
==========================================
  Hits         1138     1138              
- Misses         86      159      +73     

Impacted Files	Coverage Δ
src/spaces/continuous.jl	65.66% <0.00%> (-29.96%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ae4532...cee2f34. Read the comment docs.

[pmbaumgartner]

@rmcsqrd @Datseris

Do you have a list of what needs to be completed before this is merged? I'm working on an ABM that needs some similar functionality right now, and would love to be able to use this. I have some time to contribute as well.

[AayushSabharwal]

Hi! After a quick glance over the source code (assuming it's functional in its current state) and the thread above, here are my thoughts:

• This would make more sense if it were in its own submodule, rather than in continuous.jl
• The example should work, and be properly documented, and tests should be written.
• There are parts of the code that are not idiomatic julia: things like having fmp_parameters_init instead of a constructor.
• All the functions also assume the FMP_Parameters struct is available in model properties as model.FMP_params which isn't ideal. Could the functions be refactored to make it so FMP_Parameters is passed in wherever it is required?
• Is there a way to better handle obstacles/targets than have them be a different type of agent?
• The code assumes the agent struct has some fields (Ni, etc.). Do these fields need to be part of the agent struct, or should they be part of FMP_Parameters? If they are only required by the algorithm, and the user doesn't need them, it's likely they can be moved.
• It would also be a good idea to bring this up to the current master, since there have been a lot of changes since this was last worked upon
• All relevant API should be documented

It would really be great to see this brought to completion, it's a really cool feature!

[rmcsqrd]

Hi @pmbaumgartner,

Finishing this has been in the back of my mind for a while - it fell off after my thesis and during the summer. Thanks for the motivation to pick this back up.

Thanks for the input @AayushSabharwal, I will use that as guidance in conjunction with the other outstanding items brought up previously in this PR. All your comments are well taken; I have some follow-up thoughts on a few of them:

This would make more sense if it were in its own submodule, rather than in continuous.jl

How about in src/submodules/pathfinding?

Is there a way to better handle obstacles/targets than have them be a different type of agent?

There is a little bit of discussion on this previously in the PR - if you look at the actual algorithm there isn't much of a difference between "agents" and "obstacles/targets". When calculating the attractive/repulsive forces for a specific agent, the agent is agnostic to whether or not it is being repulsed by another agent or an obstacle.

That said if there is way to handle obstacles/targets that is more conformant with other pathfinding modules I would appreciate if you could elaborate more on that. This PR is attempting to implement the underlying algorithm as purely as possible but I'm happy to adapt to fit any Agents.jl standards/etc.

The code assumes the agent struct has some fields (Ni, etc.). Do these fields need to be part of the agent struct, or should they be part of FMP_Parameters? If they are only required by the algorithm, and the user doesn't need them, it's likely they can be moved.

This is a good point - I'll clean up the agent struct. There are some algorithm hyperparameters that I'd like input on whether or not to expose. For example, the Φ parameter controls the repulsive force between agents. The original FMP algorithm uses a value which is reflected here - theoretically a user may have an interest in modifying this value but that is probably not a common use case. I tend to err on the side of exposing everything (while also providing the default value) but understand this can be cumbersome. Do you have thoughts on this?

[pmbaumgartner]

@rmcsqrd Glad you're still thinking about it and willing to move things forward. Based on the list @AayushSabharwal created, are there places you feel comfortable delegating some of that work to me? Or does it make more sense for you to complete this wholesale? For context I'm pretty comfortable with the Agents.jl API and where things should go to be consistent, and less comfortable with all the awesome math going on behind the scenes with an algorithm like this.

Is there a way to better handle obstacles/targets than have them be a different type of agent?

My thought here is that targets should work the same way that they do with the regular pathfinder (i.e. an in-bounds point passed to set_target!), and obstacles could be implemented in a similar way to how the walkmap is created with a pathfinder in continuous space. I haven't reviewed the current obstacle implementation to make a comparison, but that would be a way to make things consistent with the existing API.

[AayushSabharwal]

How about in src/submodules/pathfinding

Sounds great!

That said if there is way to handle obstacles/targets that is more conformant with other pathfinding modules I would appreciate if you could elaborate more on that

The existing pathfinding only supports static obstacles, so they're "baked" into a grid, which tells whether that grid cell can be walked on or not. I imagine this doesn't work for FMP, since you need to calculate forces from each obstacle. If the obstacles are static in FMP, maybe store them in the FMP struct? If the obstacles or targets move too, then having a flag in the agent struct is the way to go.

My thought here is that targets should work the same way that they do with the regular pathfinder (i.e. an in-bounds point passed to set_target!), and obstacles could be implemented in a similar way to how the walkmap is created with a pathfinder in continuous space. I haven't reviewed the current obstacle implementation to make a comparison, but that would be a way to make things consistent with the existing API.

If this works with the algorithm, it's probably a good idea. Having a consistent API simplifies things significantly, and minimizes the names we have to export.

This is a good point - I'll clean up the agent struct. There are some algorithm hyperparameters that I'd like input on whether or not to expose. For example, the Φ parameter controls the repulsive force between agents. The original FMP algorithm uses a value which is reflected here - theoretically a user may have an interest in modifying this value but that is probably not a common use case. I tend to err on the side of exposing everything (while also providing the default value) but understand this can be cumbersome. Do you have thoughts on this?

If hyperparameters are agent-specific, they're probably better off in the agent struct. If they're global, they should be in the parameters struct. However, any agent-specific data that the algorithm needs but the user shouldn't modify (like agent paths in A* pathfinding) should be in the parameters struct too. It feels like the Ni and Gi fields fall into this category (please feel free to correct me if I'm wrong), where they contain agent-specific data required by the algorithm, but not something the user needs to see or modify. If so, they should be in the parameters struct.

---

Another idea that came to my mind:

Instead of iteratively calculating the force between every pair of agents/obstacles/targets manually, could this be done using ModelingToolkit.jl/DiffEq.jl using the integrator interface? I'm curious as to whether this might simplify the implementation and/or improve performance, since it would only require creating the requisite equations. That said, it also raises questions of how to handle adding/removing agents in the middle of the simulation. It might be possible using the callbacks API.

That said, it's just an idea. If this ends up raising more problems than solutions, it's probably better to finish the implementation as it stands, and explore this later. If you, @Datseris or @Libbum have any thoughts on this I'd love to hear them.

[Datseris]

Hey guys! Love to see the progress on this one. Sorry that I've been inactive here!

Unfortunately I am currently on vacations for a month or so, and unlikely I will provide useful feedback here during this time! But @AayushSabharwal I trust you to guide things around and help with progressing this! When time is ripe for a "final review and merge" I can try to come back!

[AayushSabharwal]

I trust you to guide things around and help with progressing this!

Sure, I'll do my best.

---

Another thing to look into: Julia supports unicode variable names, so variables such as rho might be better off as ρ (\rho<tab>).