Refactor `AgentsVisualizations` extension
Closed this issue · 17 comments
https://github.com/JuliaDynamics/Agents.jl/tree/main/ext/AgentsVisualizations
The visualisation code works but it's of course far from perfect. Most notably, it relies on some hardcoded pieces (e.g. regarding the supported spaces) and cannot easily be extended by users to work with their custom spaces.1 I'm willing to take up this task and try to improve the visualisation code. But I would like to hear your ideas on this topic and maybe get some support from you as well.
Background
I think it makes sense to look at a bit of history to explain why it is how it is: During the time of writing, Agents.jl was very different from now (most notably a lot smaller) and the plotting code was not intended to capture all use cases that the users might have. Indeed, what we have now is more or less a big refactor that I once contributed to InteractiveDynamics.jl which has been copy-pasted over to Agents.jl at some point in the form of an extension. The visualisation code as we have it now was only (i) a reproduction of the previously existing functionality in the form of a Makie recipe and (ii) a more convenient way to quickly plot and inspect our ABM. Accounting for other spaces outside the currently supported ones wasn't really a goal, even though I was very much aware of that possibility back when I wrote the recipe (there are explanations in the docs regarding custom plots and data visualisation).
Yesterday evening the visualisation code was again called out by @Datseris over on Slack:
in all honesty the plotting code is a big quality dropdown from the normal src code of Agents.jl, so we need to re-format it at some point....
So it seems as if it's a piece of code in the Agents.jl package that he really dislikes. It's a bit hurtful to read this in public and directed towards someone else because the "big quality drop" wasn't communicated to me when I added the code in the first place (or later). Still, I personally like the visualisation code overall because there's a lot of functionality in there (buttons, parameter sliders, automatic lifting and updates, mouse-over inspection, etc.). Scrapping it completely and starting from scratch doesn't seem sensible to me. It's just, as I mentioned above, that I never had the intention to code a highly modular and abstract interface for lifting and inspection. It was only meant to allow easy plotting with a bit of functional styling and customisation.
Refactor
We're all aware that users want to create plots with a simple call to abmplot. So what needs to be done at the very least to improve the extension and make this and other custom plots possible?
- Decouple the building blocks inside the recipe from one another. Simply put, there has to be an opportunity for users to create an
ABMObservable, create a custom plot (without usingabmplot!), add controls (i.e. buttons and sliders), and add some plots for the collected data to the same figure. To be honest, I think we're mostly there but especially the controls block is not accessible by the users. - Implement a user-facing API for extending the
abmplotfunctionality with proper support for custom spaces. There are likely three crucial aspects we need to deal with:- Should there be any pre-plotting?
- How to extract space size/dimensionality?
- How to map agent positions to the points of a scatter plot?
- Document how to create own plots using this API.
That's of course not all and we need to ask ourselves another question: What needs to be done at the very best to improve the extension?
Well, that depends on how abysmal you think the visualisation code really is. I'm looking forward to your constructive ideas in this thread. :)
Footnotes
-
There's a very clear tendency of users to misunderstand custom spaces. They often incorporate model properties into their custom space structs and most often those custom spaces are just a
GridSpace/ContinuousSpacewith a bit of sugar on top. Ultimately I think that the arising issues from custom space plotting are perfectly solvable by just telling people to not overload the functionality of their model'sspacefield and stick to existing spaces. But we cannot solve this often-occurring misunderstanding, except for explaining over and over again what exactly a space is intended to do and what not. So this issue - and the PR that will follow from it - are there to allow for this because, honestly, who are we to tell people what to do and what not? ;) ↩
to be clear, I was mostly referring to myself when i said that the code is a large drop in quality, as I wrote the first version so I have the majority of the blame when it comes to design
as far as I can tell the problem with current code is that unlike src Agents.jl it isn't modular. Most things are hardcoded. Functionality can be detached into declared API that is extended by spaces and models. This functionality can documented and exported and then others can extend it as well. Same line of thinking for adding a new button: define the function necessary and how to add it and then it can be extended.
So, pretty much what you wrote here as a solution 👍🏻
to read this in public and directed towards someone else
I never directed my comment to anyone, and as far as I can tell in this thread it was only me and a user trying to extend the functionality. I am sorry you feel hurt by this. As I said above, I was thinking of my own work. I will be making this more clear in the future; I am direct and If I have a problem with someone I make it very clear to that someone. I appreciate any contributor to projects I also contribute!
- There's a very clear tendency of users to misunderstand custom spaces. They often incorporate model properties into their custom space structs and most often those custom spaces are just a
GridSpace/ContinuousSpacewith a bit of sugar on top.
I've noticed the same so we should add notes about this in the Dev Docs about how to extend space. Something like "think about if you really need to extend space. space is only about the location of agents and their movement."
In my opinion even if the the code of AgentsVisualizations has some issues, it is nonetheless great. It is far from easy to write a Makie interface like this. At the moment I'm a bit busy with some backlog, and so when I find the time I will direct my efforts to finish to code the last bits to release v6.0. But I would love to see a more general rewriting of the visualization functionalities!
I'm currently working on this and am making great progress so far.
While writing a more extensible API for custom spaces, I'm refactoring the recipe quite a bit in the hopes of simplifying the interface overall. Hence, I would probably like to make this PR slightly breaking. (@Datseris please look away.) Of course I will add some deprecation warnings to cover those users who don't read the release notes and/or update notifications in the REPL. The recipe will continue to work as before (so it's not really breaking per se) but the preferred usage will change a bit.
@Tortar So if it's somehow possible, please wait with the 6.0 release until my work here is finished. It will go nicely hand in hand with that big major release. :)
Sure, more a major release is big the better!
But shouldn't you first present what the new design is, so we can talk on that here, before implementing all the breaking changes first and then present the design? This way, if there are alterations to be done, it saves you the effort of doing them twice!
before implementing all the breaking changes
There will be no breaking changes.
present what the new design is, so we can talk on that here
Of course, no problem. To be honest, I thought that what we discussed above was enough for me to get started with the actual coding. Today I had some time, so I took the opportunity.
There is now a more general interface for plotting. The extension files have been reorganised according to space types. The necessary plotting methods for the different spaces are now gathered in separate files and extend the base plotting functionality for abstract spaces. There is no more arbitrary requirement of defining a set of supported spaces like we did before.
There is now also a convenient checking function that triggers when somebody uses a custom space. It tells the user which new methods for the API functions are necessary or optional and if the method is usable in its current state. If the necessary methods aren't there, it throws an error and points towards our documentation.
All of what I've described here is already done and I've tested it with the examples provided in our repository's example folder. Schelling, flocking, and zombies models all work fine.
What's still missing is the following:
- Separation of the functions adding the control panels, e.g. buttons and sliders
- Refactoring of inspection code into the separate files, both for the abstract visualisation interface as well as per space
- Documentation of the public API, i.e. which functions require a method extension for user defined spaces and which method extensions are optional depending on the needs for the space
- More extensive testing, especially with some kind of custom space.
The last point will be the toughest thing to do because I don't actually have any idea for a custom space that would make any sense in the tests. Maybe you have some ideas? Otherwise I will just re-implement something like GridSpaceSingle but with a different name to test the custom space plotting API.
I don't have a better idea for the custom space test, alias of GridSpaceSingle seems good, in the end tests are always artificial
Oh, and how do we want to deal with Nothing space models? Those are not generically plottable.
My personal preference would be that by default the abmplot recipe throws an error for model::ABM{Nothing} as input. Users can then define custom specialised methods for this type signature in accordance with the public API, just like they would do for custom spaces. This approach would cover (i) the basic fact that Nothing spaces cannot yield any generic plot, and (ii) the option for users to define how their Nothing space model should be plotted without having to define a custom space. Would you be OK with this approach?
Exactly. When calling abmplot(model::ABM{Nothing}) we would only produce a set of buttons. Not really helpful, but maybe also just what people might expect if they don't provide parameters for sliders?
One more thing that we should discuss: In the current version we have multiple plot kwargs NamedTuples that get passed around inside the recipe. Where exactly they get used in the recipe is pretty opaque to the users. This should be cleaned up and I have the following proposal:
Current set of plotkwargs (see here):
scatterkwargsosmkwargsgraphplotkwargs
Both scatterkwargs and graphplotkwargs are used to plot the agents. The latter is used for GraphSpace models and the former is used for everything else. osmkwargs are used to style the osmplot! space-dependent preplot for OpenStreetMapSpace models.
The proposed new recipe structure in PR #934 lists four plot layers that get constructed (see here): spaceplot!, heatmap!, static_preplot!, agentsplot!
Therefore I would like to change the kwarg NamedTuples that abmplot takes to reflect these plotting layers:
scatterkwargsandgraphplotkwargsget merged intoagentsplotkwargsosmkwargsbecomesspaceplotkwargs
Which kind of attributes these kwargs can take depends on the space type of the model to be plotted. This will be described in the docstring. The deprecation of the three old sets of kwargs can be easily captured inside the recipe and users can be notified that this is the old way and they should use the two new sets of kwargs instead. Hence, the proposed change is not breaking.
The new structure of the kwargs attributes will be a lot more generic and allow for users to pass in their appropriate keywords for their custom spaces, all while knowing which set of kwargs goes where. What do you think about this? Happy to hear your thoughts or different ideas for handling it. :)
In my opinion your design is good since having different kwargs for different space+agents plotting wasn't a good choice, while merging scatterkwargs and graphplotkwargs in one, given that they both operate on agents is better. My only question is: do we have some keywords to be used for spaceplotkwargs when used in a non-custom space different from OSMSpace? But in any case the rewording seems good to me since it conveys the idea to pass there keywords for a custom space.
yes, I also agree with this refactor.