Refactor recipes

[fbanning]

• [x] Refactor recipe system to dispatch on model.space
• [x] Rename and reorganise files in /src/agents/ to better reflect what they're doing and keep them more functionally separate from each other
• [x] Refactor OpenStreetMapSpace into recipe system with osmplot as static_preplot! and agents as scatter! plot
• [x] Remove ABMStepper struct and functionality and move all of it into Attributes of ABMPlot recipe
• [x] Move all former functionality of abm_plot, abm_play, and abm_data_exploration into ABMPlot recipe
  • [x] abm_plot
  • [x] abm_play
  • [x] abm_data_exploration
• [x] Update abm_video convenience function to work with ABMPlot recipe
• [x] Add inspectable::Bool Attribute to recipe which controls if agent inspection on mouse hover should be enabled or not
• [x] Add convenience function akin to former abm_data_exploration
• [x] Test all examples without and with controls, without and with data plots, without and with inspection
  • [x] flocking (2D continuous with polygons)
  • [x] daisyworld (2D grid)
  • [x] schelling3d (3D grid)
  • [x] zombies (osm)
• [x] Documentation. All of it.
• [x] Update Makie compat entry to 0.16. (Currently blocked: We depend on OSMMakie and subsequently on GraphMakie which in turn waits for the release of Makie 0.16.4 that fixes one of their bugs. Need to be a bit patient.)

[Datseris]

@fbanning can you explain to me something which I don't get from the recipe system... There is both a function abm_plot but also abmplot...? Why?

The other thing I don't understand is where is abmplot actually defined. It isn't here: @recipe(ABMPlot, model) do scene, it isn't here: function Makie.plot!(abmplot::ABMPlot{<:Tuple{<:Agents.ABM{<:GridOrContinuous}}}),. Like, we are calling a function abmplot! but I don't find its definition.

[fbanning]

@fbanning can you explain to me something which I don't get from the recipe system... There is both a function abm_plot but also abmplot...? Why?

The similar names abmplot and abm_plot are a bit unfortunate, yes, but couldn't really be avoided because I definitely wanted the underlying recipe to have a "telling name" (ABMPlot as its type and abmplot/abmplot! as the corresponding functions).

While abmplot and abm_plot are similar by name, their purpose is still quite different. I'll try to explain what they do:

abmplot is exported via the recipe system and takes in model<:ABM as an argument. You can see it as a new plot type which is comprised of some other plot types or primitives (e.g. multiple lines in the case of linesegments). So in the most basic case of a 2D GridSpace calling our abmplot function will just create a scatter plot of the model with the given agents positions (normally that would be pos = abmstepper.pos).

abm_plot is what already existed before and does not only return the figure but also the abmstepper. This is necessary to update the plots. If users would only use something like abmplot(model; pos = [model[id].pos for pos in ids]), the plots won't update. But you know all of this because this behaviour is just how it always was. Users are advised in our docs to use abm_plot, abm_play, and abm_data_exploration because these functions conveniently construct the ABMStepper from a given model, connect it to the newly created plot and return both the figure and abmstepper for later use.

The other thing I don't understand is where is abmplot actually defined. It isn't here: @recipe(ABMPlot, model) do scene, it isn't here: function Makie.plot!(abmplot::ABMPlot{<:Tuple{<:Agents.ABM{<:GridOrContinuous}}}),. Like, we are calling a function abmplot! but I don't find its definition.

That's just the way Makie recipes work: They have two parts. You need to define both a @recipe block for the plot type and its attributes as well as a custom plot! function to define the actual functionality of the resulting plot function. I really can't explain it any better than it's done here: https://makie.juliaplots.org/stable/documentation/recipes/index.html#full_recipes_with_the_recipe_macro Reading just that paragraph (up until the example) should hopefully clear things up for you. If it doesn't, feel free to ask again.

[fbanning]

Even though the above comment was already pretty long (it's really hard to explain these things without going into too much detail), let me add another thought on the first topic: It's possible that the overlapping and maybe somewhat confusing naming can be fixed. All while making our code base smaller and providing a better user experience. Too good to be true? Maybe.

In its current state, abmplot is static/unusable without first converting the model data into Observables stored in ABMStepper and linking its fields to the plot. But as I've already hinted at in the initial PR introducing recipes (see the Further Ideas section in the initial post of #72), I think it's possible to get rid of ABMStepper. Instead we could rely only on the function args/kwargs defined in the recipe because all of them are automatically converted into Observables. Their values would have to lift the values from the only argument provided to the function, namely model<:ABM.

If this is indeed possible, then some massive simplification of our code would follow. We could remove ABMStepper, its specific stepping functions as well as the need for abm_plot, abm_play, and abm_data_exploration. Instead we could handle those cases via keywords (= Attributes) of the recipe system (sth. like enable_controls = true and enable_data_exploration = true).

Then the usage would look as follows:

#formerly abm_plot
abmplot(model)

#formerly abm_play
abmplot(model; enable_controls = true)

#formerly abm_data_exploration
abmplot(model; enable_data_exploration = true)

Such a user experience would be highly desirable to me. It gives the user one single function to work with and allows custom usage via the keywords.

I want to try out this approach right after I've finished this PR but so far I didn't find the time or muse to try it out. But let's do this step by step. Let's first finish this PR. Then I'll work on the next step to (hopefully) simplify our plotting a bit more.

[Datseris]

Thanks for the detailed reply, I understand what is going on now. Notice that the necessity of ABMStepper comes not only from creating the observables and carrying them around as a container, but also because of Julia's multiple dispatch system. You need a way for the central step! function to trigger the observable updates without extra lines from the user, and this is exactly what step!(::ABMStepper d0es.

EDIT: But, given the recipe system, we can merge the structs ABMPlot and ABMStepper, right? We need the later to define dispatch on step!, and we need the former to define dispatch on Makie's plotting system, so in essense we need 1 struct that defines 2 dispaches?

EDIT 2: But, we should be careful: if this is indeed possible, we should only do it if it increases code clarity. The important point is not to minimize lines of source code, but to maximize how easy it is to understand the code. Sometimes the extra lines help with that.

[Datseris]

Refactor OpenStreetMapSpace into recipe system with osmplot as static_preplot! and agents as scatter! plot

Wait, I'm a bit lost now. The refactoring into recipes shouldn't really care about what kind of space we have right? That's exactly why I wrote the function plot_agents_space!. This is the function that dispaches on space, but I see now already that ABMPlot also dispatches on space type. But why can't it just call plot_agent_space!?

EDIT: I stand corrected. The plot recipe dispaches on the position type not the space. But, same argument: the position type from open street map space is also Point2f0, so it shouldn't care?

[fbanning]

the position type from open street map space is also Point2f0, so it shouldn't care?

I guess you're right. Will merge the two methods.

We will definitely need a new method for GraphSpace later on though because it handles things so differently.

[fbanning]

given the recipe system, we can merge the structs ABMPlot and ABMStepper, right?

I think so, yes.

We need the later to define dispatch on step!, and we need the former to define dispatch on Makie's plotting system

Yes, that's the status quo. My hope is that we can eliminate the need for a separate ABMStepper struct completely. This should be possible if we define all the necessary ABMStepper variables as attributes inside the recipe via lift. We should then be able to only update the value of the model argument (abmplot.model[] = model) after every step and the plot should update accordingly. Remember though, this is untested and just makes sense in theory in my head right now. Time will tell if it's really doable.

we should only do it if it increases code clarity. The important point is not to minimize lines of source code, but to maximize how easy it is to understand the code. Sometimes the extra lines help with that.

Absolutely agree with this but in addition I want to emphasise the importance of a good user experience. Let's see how this turns out once I start working on it. But first let's finish this PR.

[Datseris]

Yes, GraphSpace needs to be handled differently, but that's a different PR.

abmplot.model[] = model

That seems like a great approach. Lifting everything from a model observable, including the position and all other stuff. Do you think you can do this in this PR? I'd suggest to do this in the ABMSTepper first, while still keeping a separate ABMPlot, as is right now. Then, after we have this large convenience change of only having to update abmstepper.model[] = model, instead having to call step!(abmstepper, ...), we can think about how to merge the stepper and plot objects.

Regarding user experience: I think your suggestion is anyways simpler for a user. It also gives them the possibiolity of passing a new model instance instead of having to recreate a plot from scratch. Also, it simplifies a lot our "reset" button functionality.

[fbanning]

OK, I'll try to already add it to this PR. Working on it.

[fbanning]

Major update today:

• All past functionality of abm_plot and abm_play has been merged into ABMPlot recipe.
• abm_video is also working again and is blazing fast.
• abm_data_exploration is not yet implemented in the recipe. I will likely work on that over the course of this week.
• I've removed the old functions and merged their docstrings into one for abmplot. I've also updated the keywords listed there and did some overall cleanup of the docstring. It's starting to look nice again.

@Datseris Feel free to try out the new daisyworld and flocking examples from their files. I've updated them to work with abmplot recipe where possible. :)

[Datseris]

@fbanning hi, happy to have a closer look here. I see you did a pretty heavy code refactoring, can you please give me a short summary of what you refactored and where, so that it is easier for me to navigate through this PR and examine the source in detail?

[Datseris]

I'll try to work on this over the weekend!

[Datseris]

@fbanning there was one last thing I forgot to say in our call... The naming: now there is one name that is without underscores abmplot and the rest are with abm_video. I'd tend to favour abm_plot, as it naturally leads to abm_data_exploration or abm_video. The rest of the library names (besides Agents.jl) also favor underscores,.

[fbanning]

The name is automatically generated from the ABMPlot type through the recipe system. I don't know if it can be changed. I will look it up.

[fbanning]

@Datseris When locally building the docs, I get a few warnings regarding docstrings of used modules:

Warning: 266 docstrings potentially missing:
│ 
│     Agents.kill_agent! :: Union{Tuple{A}, Tuple{S}, Tuple{A, AgentBasedModel{S, A}, Agents.Pathfinding.AStar}} where {S, A<:AbstractAgent}
│     Agents.random_empty :: Union{Tuple{AgentBasedModel{<:Agents.DiscreteSpace}}, Tuple{AgentBasedModel{<:Agents.DiscreteSpace}, Any}}
│     InteractiveDynamics.darken_color :: Union{Tuple{Any}, Tuple{Any, Any}}
│     Agents.add_agent_pos! :: Tuple{AbstractAgent, AgentBasedModel}
│     Agents.genocide! :: Tuple{AgentBasedModel, Integer}
│     Agents.genocide! :: Tuple{AgentBasedModel, Function}
│     Agents.genocide! :: Tuple{AgentBasedModel}
│     Agents.genocide! :: Tuple{AgentBasedModel, Any}
│     Agents.Pathfinding.CostMetric
│     InteractiveDynamics.abmplot! :: Tuple
│     DynamicalBilliards.SplitterWall
│     Agents.grid_space_neighborhood :: Tuple{CartesianIndex, GridSpace, Real}
│     Agents.grid_space_neighborhood :: Union{Tuple{D}, Tuple{CartesianIndex, GridSpace{D}, Tuple{Vararg{Real, D}}}} where D
│     Agents.get_spatial_index :: Union{Tuple{D}, Tuple{T}, Tuple{Any, AbstractArray{T, D}, AgentBasedModel}} where {T, D}
│     DynamicalBilliards.to_bcoords :: Tuple{AbstractParticle, Obstacle}

and so on and so forth. Is this normal or am I doing sth wrong?

[Datseris]

You can safely disregard all of these warnings about missing dosctrings.

[fbanning]

Convenience function is back in working order (see convenience.jl). I've also added a bit of documentation today. OSMMakie is now also in the dependencies of the package (feels weird to write "0.0" in the compat entry but that's how it is for now).

[Datseris]

We're still waiting for GraphMakie here, right? (By the way, why does OSMMakie depend on GraphMakie? I thought you manually plot all lines based on lon-lat coordinates)

[fbanning]

We're still waiting for GraphMakie here, right?

Yes.

(By the way, why does OSMMakie depend on GraphMakie? I thought you manually plot all lines based on lon-lat coordinates)

It was the easiest way to get started. Otherwise I would have recreated 80% of the functionality provided by GraphMakie in OSMMakie. This is not very economical to get a first version out. In the long run it might turn out that dropping GraphMakie as a dependency rolling an own solution might be worth it. Right now, it's better to leverage the power of GraphMakie. :)

[Datseris]

@fbanning I'm a bit confused with the abmplot as I'm trying to work on this PR as I've been doing a lot of other improvements to the library in the meantime. The docstring of abmplot seems to also be merged wit hthe docstrong of the previous data exploration app. E.g., In line 70 it starts:

### Axis related
* `ax = nothing` : The axis from which  # TODO: This sentence is incomplete
* `aspect = DataAspect()` : The aspect ratio behavior of the axis `ax`.

# Interactive application

Launch an interactive application that plots an agent based model and can animate
its evolution in real time. Requires `Agents`.

The two functions `agent_step!, model_step!` provided as keyword arguments will decide how 
the model will evolve, as in the standard approach of Agents.jl and its `step!` function.

Could you please clarify the documentation string a bit? Or add a brief "tutorial" on how to make custom animations in the agents.md documentation page? I'm sorry, it has been some time and I've lost touch with this PR.

[fbanning]

The docstring of abmplot seems to also be merged wit hthe docstrong of the previous data exploration app. E.g., In line 70 it starts:

No, I've split the docstring up. The docstring of abmplot only explains the functionality of the interactive application when passing agent_step! and/or model_step! as kwargs. Inside convenience.jl there's still the rest of the old abm_data_exploration docstring which explains how the scatterline plots are created via adata/alabels and mdata/mlabels.

Happy to improve the docstring if you can tell me a bit more precisely what's unclear to you.

[fbanning]

And btw after you merged master into this branch, the tests all fail because the requirements cannot be satisfied. This is completely as expected and due to the last to-do I've listed in the first post of this PR. I will quickly update the dependencies and then hopefully everything will work as expected.

[Datseris]

Don't worry about failing tests at the moment. Will get back to the explanation on the weekend where I will work on this again. If you want you can start composing the new documentation page (simply walk a user through how they would make their own animations, just like you do in the example). I'll do this on the weekend otherwise.

(I have forgotten that now all interactivity-related functionality is already in abmplot)

[fbanning]

The transition to Makie 0.16 should be done now. GraphMakie and therefore also OSMMakie now use it.

[Datseris]

Okay, then you'd need to increment versions in the Project.toml accordingly. Are you going to write the documentation page in the end? I won't have time to do direct contributions here until after the 16th of march.

[fbanning]

Yes, I'll write some docs soonish.

[fbanning]

Today I've worked on the docs a bit. Still need to rework the custom plots section for the new recipe system, then I think I'm done with the docs related to this PR. Maybe I'll also add one very straightforward "example" section to the page with commented code. Not sure yet.

[Datseris]

You can get inspiration of how I've re-designed the documentation for the plotting of DynamicalBilliards.jl, where it also follows a similar idea of creating some core observables and then stepping over them. https://github.com/JuliaDynamics/InteractiveDynamics.jl/blob/master/docs/src/billiards.jl

[Datseris]

I am hoping to merge this in today.

[Datseris]

@fbanning the axis limits are wrong in the OSM plotting recipe. That is what I see once I run the zombies example:

[fbanning]

This is a bug from GraphMakie. It should be fixed soon(tm). I'll ask the dev how far he got with this.

In the meantime you can pass the keyword graphplotkwargs=(; curve_distance = 0) to the osmplot function call in the recipe to create correct autolimits for the purpose of creating an example map png.

[Datseris]

@fbanning I am genuienely confused... It seems to me now that the previous abm_data_exploration functionality is contained fully within the abmplot function. So what's the reason of existing for abm_data_exploration in the file convenience.jl?

[Datseris]

no, i think i am starting to understand...? The only thing the abm_data_exploration does is that it also makes the data plots, right?

[fbanning]

no, i think i am starting to understand...? The only thing the abm_data_exploration does is that it also makes the data plots, right?

Exactly, that's what you wished for in our video meeting some weeks ago. :)

[Datseris]

I am asking you some quick questions at Slack because I've apparently lost touch with the what we had discussed... we should have written it down :D

[Datseris]

BTW Ive just noticed that when I call abm_video on schelling, it correctly plots the Schelling model with data aspect ratio, but simply calling abmplot does not have data aspect. For GridSpace we should have data aspect I think.

Oh yeah that's the old "ax needs to be a keyword" problem.

[Datseris]

@fbanning we need to test this new API also with a model that creates Agents. I've run it with a case of the bacteria growth, where new agents are being created, and I immediatelly got the error:

ERROR: LoadError: Metadata array needs to have same length as data.
                    Found 200 data items, and 100 metadata items
Stacktrace:
  [1] error(s::String)

I think this happens because each observable update triggers a plot update and hence once a new agent is created the scatterplot has 3 positions and 2 colors (if initially we had 2 agents).

EDIT: No that is not the problem. I also tested now with the Fractal Growth, that also makes new agents, and that one worked without a problem. Bacterial Growth is the one that uses Polygons as markers though, and maybe there is a bug there. Will examine more.

[Datseris]

There is somethjing that is inconvenient about current state: if we step the model observable, the dataframes are not updated. So the user would have to do it manually. If the model of the "recipe object" is stepped when running the the model via the buttons of the interactive app then the dataframes are updated, but that's not how it is supposed to be for custom plots.

I propose an alternative:

Define step!(abmplot::_ABMPlot, args...; kwargs...). This function steps the model, then checks if data should be collected, and then would collect data. The only thing we have to add is a step counter inside the plot recipe, so that this keeps track of current step, and is also set to 0 on reset. This will actually simplify the source code a bit, because we will need no functions to do "data collection". Everything will be working from the lifted observables. What do you think?

[Datseris]

Actually, it gets even better. Defining a "model obserrvable" struct that contains all stepping related information is the best way to go I think. It defines a simple step function that the source code of the ABMPlot can utilize and become smaller.

This has several additional benefits: It clarifies the API a lot. There is no reason for a user to obtain a "plotted object" from the Makie ecosystem. What we want is that the user gets the model and dataframes observables. Instead the user has this model observable struct whose fields are the observables the user needs.

[Datseris]

This interface seems much cleaner and detaches interaction from Makie recipes, which the frontend user shouldn't really care about. Unfortunately my first train ride is already over. I'll finish this in my second ride tomorrow. If you have time to have a look let me know if you have any feedback.

[Datseris]

(by the way, I've followed here exactly the same design I used for DynamicalBilliards.jl in InteractiveDynamics.jl, so this design is already tested to work very nicely, see e.g., https://juliadynamics.github.io/DynamicalBilliards.jl/dev/billiards_visualizations/ )

[Datseris]

Nice, I'm finished! Small things that remain:

• [x] to build the docs locally and confirm everything works
• [x] Record new gif with new app and upload to website for linking
• [x] also build same docs in Agents doc
• [ ] Release InterDyn next version

@fbanning this is a TODO for you I guess (but also not a priority for merging and tagging): the inspection pop up does not work at the moment. Maybe I am missing an obvious mistake/bug or so, but I couldn't get it to work.

[fbanning]

OK, everything seems to work now. I'm locally building the docs to double check that everything looks good.

[fbanning]

All fine. Tag, merge, party.

[AayushSabharwal]

I haven't had the time to keep track of much here or in Agents for a while now... Looking forward to playing with the new plotting once it's out