Refactor visualisation extensions

[fbanning]

Closes #914 , #925, #930, #951

Preface

This PR is relatively big, I know. But this refactor was really necessary and kind of long overdue. So, sorry but not really all too sorry, I suppose? ¯\_(ツ)_/¯

Description

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. -- quoted from https://github.com/JuliaDynamics/Agents.jl/issues/914#issuecomment-1808964944

The space visualisation API is described in space-visualization-API.jl. Users can easily copy this file and extend the functions to work with their custom space.

This whole PR is supposed to be not breaking. Where I've had to make some changes that could be seen as breaking, I've tried to handle them gracefully so that the user gets notified of any deprecations while their code still continues to work as it did before.

Things left to do

• [x] Separation of the functions adding the control panels, e.g. buttons and sliders (add_interaction!)
• Refactoring of inspection code
  • [x] Put all the space-dependent functions into separate files, both for the abstract visualisation interface as well as per space
  • [x] Define missing inspection functions for GraphSpace and OpenStreetMapSpace
• [x] Create convenient alias for space dependent _ABMPlot
• [x] Remove unneeded type annotations
• Merge plot kwargs and handle deprecations
  • [x] scatterkwargs and graphplotkwargs become agentsplotkwargs
  • [x] osmplotkwargs becomes spaceplotkwargs
• [x] Add abmplot kwarg to disable custom space checks
• [x] Finalize custom space checks function once the public API is fixed
• More extensive testing
  • [x] custom space
  • [x] nothing space
  • [x] 3D plots
  • [x] flocking example with polygons
• [x] Handle agent position offset differently so that inspection continues to work
• [x] Extend visualizations test suite to a very basic implementation matching the models used to test this PR
• [x] 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
• Makie v0.20 related things:
  • [ ] Fix 3D ContinuousSpace inspection
  • [ ] Polygon plots are wonky/broken and run very slowly
  • [x] Unify function arguments for agentsplot! and spaceplot!

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 43.17460% with 179 lines in your changes are missing coverage. Please review.

Project coverage is 79.18%. Comparing base (8b5b456) to head (20bb280). Report is 80 commits behind head on main.

:exclamation: Current head 20bb280 differs from pull request most recent head 640970f. Consider uploading reports for the commit 640970f to get more accurate results

Files	Patch %	Lines
ext/AgentsVisualizations/src/spaces/abstract.jl	21.69%	83 Missing :warning:
ext/AgentsGraphVisualizations/src/spaces/graph.jl	41.66%	28 Missing :warning:
ext/AgentsVisualizations/src/spaces/nothing.jl	0.00%	18 Missing :warning:
ext/AgentsVisualizations/src/spaces/continuous.jl	0.00%	16 Missing :warning:
ext/AgentsVisualizations/src/utils.jl	46.15%	14 Missing :warning:
ext/AgentsVisualizations/src/abmplot.jl	85.71%	7 Missing :warning:
ext/AgentsVisualizations/src/spaces/grid.jl	66.66%	5 Missing :warning:
src/spaces/openstreetmap.jl	0.00%	4 Missing :warning:
...gentsOSMVisualizations/src/spaces/openstreetmap.jl	90.00%	2 Missing :warning:
ext/AgentsVisualizations/src/interaction.jl	83.33%	1 Missing :warning:
... and 1 more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #934 +/- ## ========================================== + Coverage 70.12% 79.18% +9.06% ========================================== Files 42 49 +7 Lines 2718 3051 +333 ========================================== + Hits 1906 2416 +510 + Misses 812 635 -177 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Tortar]

This is awesome...will take the time to review soon! (but I'm not that good at plotting stuff in Julia so I think I can't suggest that much)

[fbanning]

Thanks for giving a review a shot, @Tortar , much appreciated.

Indeed, it's not that important that you look at all the details of the implementation. I would say that I'm already relatively deep into the Makie plotting rabbit hole, so I hope that you can put some trust into my plotting code.

Instead it would be especially nice if you can have a look at the structural aspects of the PR. For instance, the definition of the API, which things would need special attention when creating the documentation, if I wrongly associated any type annotations, or whatever. :)

[fbanning]

Oh, and here are the two files that I've used to test the PR: CustomSpace.txt test-AgentsVisualizations.txt

You need to rename them to .jl because GitHub is just being GitHub again and only allows .txt files to be directly uploaded... 🙄

Put them into a separate folder and create a new Julia environment. For the setup of the env you should run:

]dev Agents # with the correct branch checked out, of course
]add GLMakie, GraphMakie, OSMMakie # standard released versions

[Tortar]

I'm sure you are much more experienced than me on that, I will focus on the structural aspects you specified.

Thanks for the .txt-actually-.jl files, indeed I was asking myself how to try to visualize everything

[Tortar]

That before plotting this:

julia> custom_space_schelling()

prints

┌ Info: Checking for methods that have to be defined to plot an ABM with custom space type
│   CustomSpace{2, false}
│ === Required
│ ✔ agents_space_dimensionality(space::CustomSpace{2, false})
│ ✔ get_axis_limits!(model::ABM{CustomSpace{2, false}})
│ ✔ agentsplot!(ax, model::ABM{CustomSpace{2, false}})
│ === Preplots (optional)
│ ✔ spaceplot!(ax, model::ABM{CustomSpace{2, false}}; preplotkwargs...)
│ ✔ static_preplot!(ax, model::ABM{CustomSpace{2, false}}, p::ABMPlot)
│ === Lifting (optional)
│ placeholder
│ === Inspection (optional)
│ ✔ ids_to_inspect(model::ABM{CustomSpace{2, false}}, pos)

is very nice and we should probably add something similar for the other customizations interfaces in Agents.jl, but maybe a keyword to disable the checks (even if enabled by default)?

[Tortar]

nothing space currently:

What about removing the blank plot instead?

And also maybe a better layout in this case is putting just two columns of charts, followed by at the bottom sliders + buttons when the space is nothing? I don't know how much code changes this requires, maybe another PR outside the 6.0 release scope could improve this

[fbanning]

What about removing the blank plot instead?

I've wrestled with this for multiple hours straight and may have gotten a few more gray hairs because of it. A few problems with this: The axis is already created outside of the actual recipe, so we would need to circumvent this. However, without any axis, we don't have access to the scene created by the recipe and all the nice things it entails, like easy access to lifted plot properties, the abmobs, etc. Also Makie threw nasty errors at me when I just tried to leave the plot completely empty or when I even tried to delete any placeholder plots from inside the recipe. It was really quite a hassle and in the end I opted for this simple version.

Sure, it would really be nicer to just not show an empty plot for Nothing spaces. So I would prefer to postpone this change for a later release. It's really not that big of a deal right now because whoever wants to plot their Nothing space models in a specific way, has to deal with the agents visualizations API anyways. :)

[fbanning]

maybe a keyword to disable the checks (even if enabled by default)?

That's a good idea that I also had at some point along the way. Eventually I forgot to implement it. Will do. :)

[Tortar]

It is also true removing the blank plot is even somewhat unrelated to the scope of this PR, so let's postpone it. I'll open an issue about this

[Tortar]

I created sometime ago a test file for visualizations: https://github.com/JuliaDynamics/Agents.jl/blob/main/test/visualization_tests.jl, but it is just a scratch. Maybe we can substitute the tests there with your different tests for this PR? The tests could be just if the commands works for now, we will maybe add some introspections later, what do you think?

[fbanning]

Sure, I would be happy with something like that. It would be a great first attempt at a mini testsuite for visualizations.

We could also try to build a test suite similar to how GraphMakie does it. They first create persistent reference images that are then compared to the output generated by the plotting functions. Maybe we can do something similar?

Tests for interactivity are somewhat complicated to write. Although we could programmatically change the observable values for clicks of each button and then check whether the model state correctly changes. Bigger topic though, so maybe something for another PR?

[Tortar]

Yes, the good extensions you are proposing are better for another PR, in this one you could just slap your .txt-actually-jl content in the test file in my opinion :D

[Datseris]

I'll also review this, let me know when you'd like that!

[fbanning]

OK, today's commits brought back all the core functionality that we had before. This means that inspection for GraphSpace and OSMSpace are working again. As a nice addition, their implementations serve as nice examples of what users can do when they have a bit more complex custom spaces and how they can handle things inside their userspace code. :)

Feel free to give a review a shot now, @Datseris and @Tortar. As can be seen in the todos listed in the first post of this PR, there's mostly the user-facing documentation left to do. I will write this once I have received your feedback on the code so that I know its state is somewhat final.

[fbanning]

I've tested the flocking model with polygons and it works. I've also found a way to deal with the weird polygon inspection that was bothering me for a long time. It's not perfect but it's a workable hack for now. Maybe someone can come up with a better version at some point.

[fbanning]

I can also confirm that 3D visualizations work fine.

[fbanning]

One thing that we should further check is that all plotting is model agnostic. I.e., it should work out of the box with either the current StandardABM or the new EventQueueABM. For this to occur, all methods that access model should use the model accessing API. E.g., use things such as allagents(model) to get the list of agents, etc.

Afaict there is no place in the plotting extensions that doesn't use allagents/abmspace/whatever API functions.

Also ensure that we don't type annotate ::StandardABM but rather the abstract type ::ABM.

I've only used ABM everywhere.

I haven't tested whether this works yet with EventQueueABM, but we can test this after merging both this and #940 .

Yep, I think this can be done after merging this PR. If any problems occur, we can fix them separately because they're not directly related to the refactor but to an extension of model types.

[fbanning]

Just found out that how we handle offsets of agent positions is not so optimal for inspection. Will add that to the list of things that have to be fixed in this PR as well.

[fbanning]

Inspection of plots with positions with offset works for GridSpace now but it's just a workaround. I need to have a closer look at how to handle this. The problem is that offsets are generated on the fly and we don't have a way to keep track of the offset values for each agent position so that we could correct from inside the inspection function. It's nothing crucial for now and it's also not a regression due to the refactor. I'm pretty sure that this has been a bug before this PR and it was just never used by anyone so it went unnoticed.

[fbanning]

In other news: Makie v0.20 was released a few days ago which is a great release (seriously!), however it introduced a few issues.

• Inspection tooltips are now drawn below the Axis spines which is not so nice to look at. This is a Makie issue (see here) and I don't know how we could fix it on our side. Let's wait for an answer there, I'm sure they'll have an idea how to sort it out. :)
• Agent inspection of 3D ContinuousSpace models (e.g. Predator-Prey-3D) is broken. Somehow the text of the tooltips always stays empty and I don't know yet what's the issue here. Need to dig a bit deeper.
• Models with polygon plots (e.g. the flocking example) are very slow to run and the polygons are also way bigger than they were with Makie v0.19. Setting as = 5 fixes it in the short term but after running abmplot a few times, I get a weird atlas warning from Makie that I've never seen before. Need to find out how to properly replicate this behaviour and then report the bug over there.

[Tortar]

Seems like they solved the problems in Makie if I'm not mistaken!

Related to #948, can we fix the version of Makie in the project.toml to 0.20? So that we can control better that everything works before updating

[fbanning]

Seems like they solved the problems in Makie if I'm not mistaken!

Yep, it's fixed. :)

Related to #948, can we fix the version of Makie in the project.toml to 0.20?

Sure we can do that. Would make sense to do it in this PR, so I'll add that to the to-do list.

I've been on vacation the past days and haven't found any time to look at the other two issues that I mentioned after the switch to Makie 0.20. Will try to work on them soonish and if I'm unable to fix them, then it's maybe better to move them into separate issues that we can work on after this PR has been merged.

[Datseris]

sorry, i've been away for too long. Do you guys need any support from me here?

[fbanning]

Nope, thanks for asking. This PR was put aside for a while because of my absence. Right now I have to finish some more urgent work related things before being able to finalise this PR. Hope to get it through the door until February.

[Tortar]

Hi @fbanning do you happen to have some time to finish up this PR? Because we are really close to finalize everything for v6 I think, otherwise we could merge this as is, and improve the rest in some follow up PRs

[fbanning]

I don't see myself finding the time to finish this PR in the next days. If 6.0 should be released really soon, feel free to merge. The new plotting infrastructure should mostly work as far as I can tell from the stuff done two months ago. It won't be without bugs (e.g. some inspection issues I know of and also the open to-dos from the list above). But maybe it's also a good idea to start a clean PR after this has been merged to fix all the remaining issues with it (and new ones that will probably arise soon after this has been released).

Oh and I just saw there are two file conflicts that need to be resolved. Those haven't been there the last time I committed to this branch, but it's likely they can be fixed easily.

[Tortar]

thank you for all the past work @fbanning ! I think as you say that it is better to improve what needs to before v6 in separate PRs