Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor visualisation extensions #934

Merged
merged 115 commits into from
Feb 27, 2024
Merged

Refactor visualisation extensions #934

merged 115 commits into from
Feb 27, 2024

Conversation

fbanning
Copy link
Member

@fbanning fbanning commented Nov 15, 2023

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 #914 (comment)

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

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

@fbanning
Copy link
Member Author

fbanning commented Dec 1, 2023

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
Copy link
Member Author

fbanning commented Dec 1, 2023

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 Tortar mentioned this pull request Dec 28, 2023
@Tortar
Copy link
Member

Tortar commented Dec 30, 2023

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
Copy link
Member Author

fbanning commented Jan 8, 2024

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
Copy link
Member

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

@fbanning
Copy link
Member Author

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
Copy link
Member

Tortar commented Feb 25, 2024

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
Copy link
Member Author

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.

Copy link
Member

@Tortar Tortar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass on my machine so I approve. There is one conversation left but it doesn't seem blocking. We are left with the problems stemming from the update of Makie.

@Tortar
Copy link
Member

Tortar commented Feb 26, 2024

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

@Tortar Tortar merged commit c938632 into main Feb 27, 2024
3 of 6 checks passed
@Tortar Tortar deleted the visualisation-refactor branch February 27, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants