Add support for GraphSpace in abmplot recipe

[fbanning]

Closes #11

The basics are working already.

I'll try to finish this in the upcoming days so that it works nicely with user-styled vertices and edges.

[fbanning]

Some to-dos:

• [x] Hide axis labels for GraphSpace plots
• [x] Implement dynamic styling option for vertices and edges
• [x] Add SIR model to example/ for testing
• [x] Test with examples from examples/
• [x] Decide what to do regarding keywords (see comment below)

[fbanning]

Hey @Datseris , a design question for you: The GraphSpace plots behave a bit differently than the rest because we have not just agents (e.g. scatter points) like in the other spaces. Graphs consist of vertices and edges, therefore we need to allow users to style them appropriately.

My current implementation uses the ac/as/am kwargs to style the vertices of the resulting graph, as that's the behaviour that people might expect. The edges are then styled with graphplotkwargs = (; edge_color = foo, edge_width = bar) and this is handled appropriately in the recipe to account for the possibility of passing functions as usual.

Now my question is whether this seems sane to you? There's certainly a break in the UX here but I'm not sure how to solve for it. It boils down to these two options:

abmplot(model; ac = node_color, as = node_size, graphplotkwargs = (; edge_color, edge_width))
# or
abmplot(model; graphplotkwargs = (; node_color, node_size, edge_color, edge_width))

Which do you prefer? The first version is closer to the regular behaviour of our abmplot. The second version is closer to the regular behaviour of GraphMakie's graphplot but might cause confusion for users expecting to be able to use the regular abmplot kwargs.

[fbanning]

Other than that this PR just needs to add some documentation, increment patch number, etc.

Here's a little (admittedly weird) showcase of our SIR model with random colours and widths of the edges:

https://user-images.githubusercontent.com/65158285/201760261-4e0a87f5-5c1f-4d1b-8aef-bbba1ac403c7.mp4

[fbanning]

ac/as/am kwargs

Maybe it would be a good idea to drop them completely and call them color/markersize/marker as it's done in most other recipes as well? 🤔

[fbanning]

I've rearranged lifting.jl to make use of method dispatch instead of nested if/else statements for type checking inside the function body. This was done because it started to get messy with all those extra if/else statements for special cases (which space?, function or not?, etc.). It also reduced LOC, makes it easier to find the case I'm looking for while debugging, and will allow for easier addition of more special cases later on. It's a win-win-win situation I think.

[Datseris]

hi @fbanning thanks for working on this. I am having an eye surgery tomorrow and i'llb be away from the computer for a month. Just letting you know it will be a long while before I review this (and I dont have time tomorrow unfortunately...). thankfully everything seems to be working ok here so you can be using this in your projects.

quick asnwer: yes for renaming am, as, ac to the standard names as long as its done in a non breaking way and in a different pr.

i prefer that the node color size and marker is explicitly stated as keyword am, ac ,as, as it was in the original implementation of graph plotting space. the edge can be keyword propagation to the graph recipy.

[fbanning]

i prefer that the node color size and marker is explicitly stated as keyword am, ac ,as, as it was in the original implementation of graph plotting space. the edge can be keyword propagation to the graph recipy.

OK, then the current implementation does exactly that and I don't need to change anything.

quick asnwer: yes for renaming am, as, ac to the standard names as long as its done in a non breaking way and in a different pr.

This would definitely be a breaking change. I can't imagine how it wouldn't be as it would change how the recipe itself is used with new names for the kwargs. The only thing that would change would be the keywords for node styling (e.g. ac -> color). Why do you want to avoid a breaking change? This PR is already breaking as it is because I've had to refactor quite a few things in the process. That's also the reason why I bumbed the minor version number to 0.22.0 instead of incrementing the patch number.

I am having an eye surgery tomorrow and i'llb be away from the computer for a month.

Oh, fingers crossed everything will go according to plan and you'll have a speedy recovery afterwards. 🤞🏻

Just letting you know it will be a long while before I review this (and I dont have time tomorrow unfortunately...).

That's understandable, don't worry about not having the time to look at the changes.

thankfully everything seems to be working ok here so you can be using this in your projects.

Indeed, everything is working just fine and as expected (except some minor hiccups that I'm trying to get rid of soon). Say though, if you don't mind too much, I could also merge the PR and you can have a look afterwards if there's anything that you're unhappy with? I would like to continue working on the implementation of nothing spaces soon because that's what I actually need for the ABM of my current paper. Getting this PR merged first would definitely be required for that though.

[Datseris]

you can allow keyword ac = nothing, color = default and then check

if !isnothing(ac)
color = ac
@warn "keyword `ac` is deprecated, use..."
end

which is not breaking. Besides this, why is internal refactoring leading to a breaking change? adding new stuff that can plot graph space isn't breaking, this was n't working before so now it can be anything we want without it being breaking.

Why do you want to avoid a breaking change?

Because this package has users, so avoiding breaking is always desirable if possible.

[fbanning]

you can allow keyword ac = nothing, color = default and then check

if !isnothing(ac)
color = ac
@warn "keyword `ac` is deprecated, use..."
end

which is not breaking.

Ah, sure, that can be done without a problem at all. Although it will clutter the interface a bit, no? And at some point we would have to remove that deprecation warning anyways so why not sooner rather than later?

why is internal refactoring leading to a breaking change?

While I was implementing the GraphSpace plotting option, I've found that our recipe is in the need of a bit of an overhaul (e.g. removing redundant plot attributes and streamlining a few internal things). Most of these don't affect the usage of the recipe at all and just went hand-in-hand with this GraphSpace PR so I went ahead and added them here instead of in a separate PR (which would just unnecessarily complicate things imho). But I've also changed the return variables of abmplot and abmplot! such that instead of

abmplot(model::ABM; kwargs...) → fig, ax, abmobs
abmplot!(ax::Axis/Axis3, model::ABM; kwargs...) → abmobs

they now return

abmplot(model::ABM; kwargs...) → fig, abmobs
abmplot!(ax::Axis/Axis3, model::ABM; kwargs...) → abmobs, abmplot_object

Returning the axis is not necessary because fig already allows the user to easily access everything inside it. Overall this should make the whole system more flexible and also a bit more in line with how regular recipes work. This idea came from my thought that it will be useful to allow a second method for both functions where the users don't give model as an argument and let the recipe build the ABMObservable. Now it's possible to pass an existing abmobs resulting in the following methods:

abmplot(abmobs::ABMObservable; kwargs...) → fig
abmplot!(ax::Axis/Axis3, abmobs::ABMObservable; kwargs...) → abmplot_object

Long story short: I think this should be considered breaking because I've changed the return variables in the user-facing functions. If you think that this is not really breaking but only a minor change, that's also fine with me.

Because this package has users, so avoiding breaking is always desirable if possible.

I'm one of those users as well and I much prefer an updated package with clean and consistent interface than a package that sticks to previous behaviour just for the sake of it. But this boils down to how we want to deal with our versions and when to push changes into a new release.

I would prefer to stick to the notion that any package below v1.0 can basically always be changed where even fundamental things in the API are subject to change. And I also think that breaking changes are to be expected by the current users.

And if somebody prefers to use an earlier version of InteractiveDynamics.jl because they don't want to deal with any changes or because they just don't need any of the newer functionality, then they absolutely can do that by putting a compat entry in their own project's Project.toml.

To sum things up, I think it's a good thing that we're still able to make breaking changes and I wouldn't want to hold back from doing so. Happy to hear your thoughts on this. :)

[Datseris]

Why would a user want abmplot_object ?

I have a different mentality versus breaking changes, and that is that they should be weighted in gold and only done if there is a significant benefit to them. Here I don't see a benefit of not returning the axis, even though a user can obtain it in a different way, and I also don't see a benefit of returning the plot object because what would a user do with it in the first place. the API is defined in such a way that the ABMobservable is all the user needs.

With the rest of the changes I am ok with.

[fbanning]

Why would a user want abmplot_object ?

I have a different mentality versus breaking changes, and that is that they should be weighted in gold and only done if there is a significant benefit to them. Here I don't see a benefit of not returning the axis, even though a user can obtain it in a different way, and I also don't see a benefit of returning the plot object because what would a user do with it in the first place. the API is defined in such a way that the ABMobservable is all the user needs.

True, the ABMObservable is all the user needs to run the simulations. But if they want to change anything about how the model is plotted, they also need to have access to either the fig itself (when calling abmplot) or the abmplot_object (when using abmplot! on an existing axis). This is close to how Makie recipes generally work where e.g. lines returns Figure, Axis, Plot and lines! returns only Plot. I think the proposed change would make things more consistent from a user perspective. If you're totally against returning these variables, it's of course also OK with me.

---

Thinking about it a bit further, another approach would be to completely decouple creation of the ABMObservable from the plotting recipe. This would mean that if a user wants to plot their model they would do:

model = init_model(args...; kwargs...) # returns ::ABM
modelobs = ABMObservable(model, args...; kwargs...) # returns ::ABMObservable
abmplot(modelobs; kwargs...) # returns (::Figure, ::Axis, ::Plot)

# Or for the in-place version with previously constructed 
fig = Figure(...) # returns ::Figure
ax = fig[1,1] = Axis(...) # returns ::Axis
abmplot!(ax, modelobs; kwargs...) # returns ::Plot

Our convenience functions (e.g. abmexploration) still make it very easy to create an interactive app for those times when we just want to explore a model with the predetermined layout. And when we need custom plot/app layouts, we can first explicitly create the ABMObservable to then use together with the recipe.

Do you think that this might be better and even more consistent with the recipe system?

---

With the rest of the changes I am ok with.

Nice, glad to hear that. :)

[Datseris]

Thinking about it a bit further, another approach would be to completely decouple creation of the ABMObservable from the plotting recipe.

No this is something I definitely disagree with; The users can at the moment still create the observable manually if they want to, but the majority of cases only needs the observable for the plotting, so this should definitely happen automatically without the additional inbetween step requested for the user.

I dont have a problem with returning the plot object, but I am still waiting for an argument why. In a lines plot, the returned lines object can be used in e.g., a call to Legend. In our case this isn't possible. So what I would like to know is that if I am a user what could I do with the abm plot object. I don't even know its fields, and its not a documented thing anyways. So that's why I think we don't have to bother with the complexity of returning it, which would also mean documenting it and establishing some API so that this object can actually be used somewhere.

[Datseris]

But if they want to change anything about how the model is plotted, they also need to have access to either the fig itself

But that's not how we have defined the API. The choices of how things are plotted must be done before plotting, by choosing the am,ac, and the preplotting arguments. I don't see much reason to allow changing the plot types after the plot creation. In the best case things are done only one way. And here configuring how the plot looks like is done when calling the plot the first time.

[fbanning]

OK, I've let this sink in a bit and don't think it makes sense to have this discussion block the merging of this PR (which also updates Makie versions, so it's rather crucial).

I will revert the changes to the returned variables and then there should be nothing breaking to this PR anymore. :)

[fbanning]

@Datseris Please have a final look if you're happy with everything. :)

[Datseris]

I am sorry for taking so long. Yes this is good to go, but did you add something about this in the docstring of abmplot? That am, ac, as are functions of vectors of agents if the plotting function is called on a graph space model?

[Datseris]

Eh, here we have city_color(model, idx) and ac = city_color. What does this mean...? What I thought was that for the size, markerstyle, and color, if the user provides function, the function takes as input all agents in the current node. (At least, this is what we did in the pre-Makie days where a graph space recipe existed).

In any case, this needs documentation, I don't see anywhere documnted in a docstring how to change edges etc. at the moment., but all of these are possible to do as shown in the example file. Probably worth adding a subsection ## GraphSpace recipe or something?

[fbanning]

Thanks for the feedback. Yeah, you're right, I should add documentation to explain the details of how to use the recipe with GraphSpace models. A subsection definitely makes sense and I'll add that as soon as I find the time.

Eh, here we have city_color(model, idx) and ac = city_color. What does this mean...? What I thought was that for the size, markerstyle, and color, if the user provides function, the function takes as input all agents in the current node. (At least, this is what we did in the pre-Makie days where a graph space recipe existed).

The function does that internally. It iterates over space.stored_ids which is a vector of vectors of ids in this position (see line 17). The user just provides a function that does something with each of the ids in a given position as shown in the example file. Did we handle this differently in the earlier versions of the graphspace recipe?

I chose idx as argument name for the example because it's the index of a node in the graph, indicating that we are creating one color/marker/markersize per node. While I prefer the graph based vocabulary here (since it's a GraphSpace model after all), I see that this might cause confusion between agent identifier id and node index idx. Internally it can stay as it is but for the documentation and the examples in there, I think I will switch this to pos instead, just to make things a bit clearer to the users.

I don't see anywhere documnted in a docstring how to change edges etc. at the moment., but all of these are possible to do as shown in the example file.

edge_color and all other edge related keywords are done via GraphMakie.jl. This means that the user just needs to understand how to use it to also be able to use our recipe. (See lines 107-111 for how I've handled this internally.) However, I'll definitely make sure to document this properly as well and link to the GraphMakie.jl docs for further explanation on how to tweak the resulting graph plot. :)

[Datseris]

The function does that internally. It iterates over space.stored_ids which is a vector of vectors of ids in this position (see line 17).

Okay, I see. Yes, this is different as it was done before. In the original version, the approach was: as, ac, am are functions that take in an iterable of agents and output a size, color, marker. The iterable is the agents in each given node, so literally agents_in_position(pos, model) for each pos. This means if you wanted the size to be the number of agents you'd pass as = length. If you wanted the color to be the proprtion of infected you'd do ac(agents) = count(isinfected, agents)/length(agents). It seems like an approach that remains harmonious with the other spaces, because, the ac, am, as arguments are only about the agent, not about the model. Here they remain as such without accepting the model as the input.

[fbanning]

OK, I'll have to think about how to do this.

For the edges it will of course not work because they follow such a different concept (i.e. the connection between two positions). There we will still need a special approach but that's not breaking for anyone since it's really only used with GraphSpace models.

[fbanning]

OK, now it works like this

as(agents_here) = length(agents_here)

and that should be what you want, right? No more model or position or anything else. Just a function that iterates over the agents in a given position.

[Datseris]

Thanks so much for his @fbanning ! Tagging a release now!

[Datseris]

@fbanning I think you need to update the graph makie dependency in OSMMakie for us to tag a new release, see here: https://github.com/JuliaRegistries/General/actions/runs/4074308616/jobs/7019298171#step:16:186

[fbanning]

Yeah, didn't tag the new release so far. Will do that now, should be available soon ™️ .

[fbanning]

Done. Already bumped OSMMakie compat on here as well as fixing the changes needed for Makie 0.19 to work (they renamed textsize attribute to fontsize).