Fully support Agents.OpenStreetMapSpace at all apps

[Datseris]

Currently WIP. Todos:

• [x] Remove Main. preface for OSMMakie once the package is released
• [x] Start a changelog
• [x] Port example of pathfinding in examples folder and test it as plotting in 3D

Result:

[Datseris]

@fbanning I'm having a problem. On current state of PR, when trying to run the examples/daisyworld.jl, I get:

ERROR: LoadError: PlotMethodError: no abmplot method for arguments (::Observable{Vector{Point{2, Float32}}}, ::AgentBasedModel{GridSpace{2, true}, Daisy, typeof(Agents.Schedulers.fastest), Dict{Symbol, Any}, MersenneTwister}). To support these arguments, define
  plot!(::Combined{InteractiveDynamics.abmplot, S} where S<:Tuple{Observable{Vector{Point{2, Float32}}}, AgentBasedModel{GridSpace{2, true}, Daisy, typeof(Agents.Schedulers.fastest), Dict{Symbol, Any}, MersenneTwister}})
Available methods are:
  plot!(abmplot::Combined{InteractiveDynamics.abmplot, <:Tuple{Vector{Point{2, Float32}}, var"#s220"} where var"#s220"<:AgentBasedModel}) in InteractiveDynamics at C:\Users\datse\.julia\dev\InteractiveDynamics\src\agents\abmplot.jl:21
  plot!(abmplot::Combined{InteractiveDynamics.abmplot, <:Tuple{Vector{Point{3, Float32}}, var"#s220"} where var"#s220"<:AgentBasedModel}) in InteractiveDynamics at C:\Users\datse\.julia\dev\InteractiveDynamics\src\agents\abmplot.jl:31
  plot!(abmplot::Combined{InteractiveDynamics.abmplot, <:Tuple{Vector{<:Polygon{2, T, P, L, V} where {T<:Real, P<:GeometryBasics.AbstractPoint{2, T}, L<:(AbstractVector{<:GeometryBasics.Ngon{2, T, 2, 
P}}), V<:AbstractVector{L}}}, var"#s220"} where var"#s220"<:AgentBasedModel}) in InteractiveDynamics at C:\Users\datse\.julia\dev\InteractiveDynamics\src\agents\abmplot.jl:43

So, this is rather cryptic, and I have two questions:

1. How to solve it?
2. Why did we do this recipe system? Seems like a really large amount of code that offers nothing. It literally does scatter!. I should have been a bit more critical about the recipe system because as far as I can it doesn't provide anything new or useful. It replaces scatter! or poly!, but I could just write down explicitly scatter! or poly! without the need of this recipe.

[Datseris]

@fbanning everything works now but I had to remove the recipes because i didn't want to deal with this cryptic message that can only be understood by someone that already knows how recipes work. Because I remove recipes there is apparently no more the nice display on hover you've made. I'll wait for you to tell me the simple answer here on how to either enable the recipe or the hover infobox.

[fbanning]

@fbanning everything works now but I had to remove the recipes because i didn't want to deal with this cryptic message that can only be understood by someone that already knows how recipes work. Because I remove recipes there is apparently no more the nice display on hover you've made. I'll wait for you to tell me the simple answer here on how to either enable the recipe or the hover infobox.

I will read through your work on the PR later but for now let me tell you that adding the recipe system has been a very deliberate decision from my side that I've thoroughly thought through before making the PR for it. From all my understanding of how Makie DataInspector works, it is absolutely necessary for the hovering system to work properly without overwriting the tooltips for other plots (e.g. scatter points on the right hand side of abm_data_exploration).

If you want a few simple answers from my point of view:

Removing recipes will

1. remove the custom tooltips.
2. remove the standardised interface for easy plotting of Agents.jl models via abmplot.
3. removes the ability to extend on the recipe system for more complex plots with multiple layers to them, e.g. for OSMSpace.

I'm currently strongly against this step but want to hold back on a final judgment until after I've read what you've done in this PR.

[Datseris]

perfect, we keep the recipes! I just don't know how to fix them!

[fbanning]

I'll look at your code here to see where the problem was. I maybe have some time later in the afternoon. Will get back to you.

[Datseris]

@fbanning one has to simply replace the new lines I wrote:

    if user_used_polygons(abmstepper.am, abmstepper.markers)
        poly!(ax, abmstepper.markers; color = abmstepper.ac, scatterkwargs...)
    else
        scatter!(ax, abmstepper.pos; 
            color = abmstepper.colors, marker = abmstepper.markers, 
            markersize = abmstepper.sizes, scatterkwargs...
        )
    end

in line 118 of stepping.jl file, with some lines that call your recipe and do not error.

[fbanning]

Alright, I've finally found some time to look at what you did here. There's a bit of "noise" in the PR (like renaming files) but the ideas in there are certainly good and helpful. Your initiative here caught me a bit off guard and I first needed to understand what your approach was before responding in any way. To me it's also important to remember that we're not in any hurry and can safely take a few more days to sort things out and get them right. I would rather not rush a new release just to get it out there.

Anyways, I have now refactored the recipes to properly dispatch on ABM.space instead of agent positions like it did before. This makes them less restricted to "agent-centric" plots like for GridSpace and allows for other displays of the model like for GraphSpace. The recipes should also be a bit simpler to understand now, I hope, as well as allowing easier extension should we find that we want to add new spaces in the future.

GridSpace and ContinuousSpace are already back in working order. I'm currently working on adding GraphSpace support but it's not quite there yet because it doesn't have a concept of agent positions and I need to find a sensible way to connect am, ac, as with the graph vertices while keeping things as generally usable as possible. Just takes me a bit more time to finish.

Once that works, I will make a PR so that we can move on with implementing OpenStreetMapSpace. But to be honest, I think it's pretty much impossible (or at least very cumbersome) to rebase your work here on top of those changes because you made some fundamental changes like removing the recipe system.

Would you be OK with me taking some of the more structural changes from your PR here (like renaming files and updating the examples and such) and adding them on top of my working branch? This way we will already have a good starting point for adding OpenStreetMapSpace to the bunch of supported spaces right afterwards. (This approach also gives me a bit more time to properly prepare the registration and publication of OSMMakie which I'm working on in parallel.)

[Datseris]

Hi @fbanning ,

Anyways, I have now refactored the recipes to properly dispatch on

I am assuming you have done this locally?

I'm currently working on adding GraphSpace support but it's not quite there yet because it doesn't have a concept of agent positions and I need to find a sensible way to connect am, ac, as with the graph vertices while keeping things as generally usable as possible. Just takes me a bit more time to finish

Please wait. Back in the day when we had Plots.jl plotting for graphspace, our plotting was different there. The way it worked is that the functions ac, as, am were taking as an input a vector of agent ids, the agents that are in the node. Based on all the agents in the node one decide the node size, color, and marker. That makes the most sense I think in GraphSpace.

Once that works, I will make a PR so that we can move on with implementing OpenStreetMapSpace. But to be honest, I think it's pretty much impossible (or at least very cumbersome) to rebase your work here on top of those changes because you made some fundamental changes like removing the recipe system.

That sounds like an inefficient way forwards. This PR works with three spaces. It actually doesn't work with OpenStreetMap because it is not registered, but it would if it was. What we should do is merge this PR that works perfectly and you branch after the merged PR. The main recipe file I haven't really touched anyways.

[fbanning]

I am assuming you have done this locally?

No, on a branch of my fork.

The way it worked is that the functions ac, as, am were taking as an input a vector of agent ids, the agents that are in the node. Based on all the agents in the node one decide the node size, color, and marker. That makes the most sense I think in GraphSpace.

That's how I wanted to do it in the abmplot recipe for GraphSpace, yes. Maybe I misphrased something above.

That sounds like an inefficient way forwards. This PR works with three spaces. [...] What we should do is merge this PR that works perfectly and you branch after the merged PR. The main recipe file I haven't really touched anyways.

To be honest, I'm not really happy about this because I feel my concerns are ignored. But since you already merged it, I will look at what can be done with the new state of the master branch. Will take me some time again to understand it and refactor things appropriately. Will open a PR once that's done.

It actually doesn't work with OpenStreetMap because it is not registered, but it would if it was.

I'm aware of this. You already know that I will register the package once it provides a minimal working state with a few more key features implemented (like colouring ways by their type). This is heavily WIP. I cannot work any faster on it than I'm already doing right now.

[Datseris]

To be honest, I'm not really happy about this because I feel my concerns are ignored. But since you already merged it, I will look at what can be done with the new state of the master branch. Will take me some time again to understand it and refactor things appropriately. Will open a PR once that's done.

I'm confused now. So far I thought that your concerns are that I've removed the recipes, and this has the negative implication of not showing tooltips. But in any case, I certainly acknowledged this concern:

perfect, we keep the recipes! I just don't know how to fix them!

This fix should be done in a separate PR, for two reasons: (1) the code in this pr is working. I've spent a lot of time making sure that all examples in both the AgentsExampleZoo, the Agents.jl main repo and the examples folder of this repo are working and hence the current status of the code should be "saved" into master. (2) As you said yourself, rebasing here is simply too much unecessary effort.

If you think there is better action moving forwards besides merging this and starting a new PR from master, then certainly let me know! One thing I can say for sure is that my git knowledge is as basic as it can get! So, to conclude, I'm certainly sorry if you've felt ignored, I just did what I thought was the most simple way forwards.

For the GraphSpace, we are completely on the same page as far as I have understood.

I'm aware of this. You already know that I will register the package once it provides a minimal working state with a few more key features implemented (like colouring ways by their type). This is heavily WIP. I cannot work any faster on it than I'm already doing right now.

??? This seems to be coming off totally the wrong way. I'm simply stating the fact of what is happening, as if anyone actually tried to actually plot the OSM versions, it wouldn't actually work.

Although, seeing your response, it feels like you are putting a lot of artificial strain on yourself about what registering a package means. It really, really, isn't a big deal, nor you need a polished final product. I'm not telling this so that you "work faster", so please don't take it the wrong way. Of course, you are the one who decides in the end, I'm just saying this so that you don't have to be so stressed about putting work into OSMMakie.jl!

[fbanning]

If you think there is better action moving forwards besides merging this and starting a new PR from master, then certainly let me know!

No, it's one way or the other. I just thought it might be easier to do it the other way around but it's OK now. I'm slowly going over the new code on master branch and will copy over everything from my WIP branch that I deem important. Don't worry. It will just take some time. I've opened a draft PR #87 for you to see how it's going on.

For the GraphSpace, we are completely on the same page as far as I have understood.

Certainly.

This seems to be coming off totally the wrong way. [...] I'm just saying this so that you don't have to be so stressed about putting work into OSMMakie.jl!

Yeah, maybe.