Invalid argument agents_step! uncaught by abmplot.

[iago-lito]

Hello, the minor issue I'm opening here follows a few hours that we've just spent screening all our code to eventually figure out that the error was in the following line:

fig, _ = abmplot(model; agents_step! = my_agent_step_function!, model_step! = my_model_step_function!, ...)

The error above is that the true argument name must be agent_step! and not agents_step! (with a spurious s). Our bad ^ ^

This being said, the behaviour of the above erroneous invocation of abmplot was not to yield an error complaining about the typo. This is unfortunate. Instead, abmplot ran almost-fine and was just behaving as if no agent_step! function had been provided at all, which has been very confusing.

Could some sort of check be added within abmplot to not pass typos silently?

[Datseris]

Hm, that is odd that you didn't get an error even though you provided an undefined keyword... Maybe we do an endless keyword splatting in the source code that needs to be eliminated (i.e., having kwargs... always even in the lowest level function). Or maybe this is an unfortunate consequence of Makie recipes...? cc @fbanning .

[fbanning]

Hm, yes, that's unfortunate.

Catching such errors how Makie does it is not very nice, I think. For example if you write colour instead of color, you get the following unwieldy and not very helpful stacktrace:

julia> lines(1:10; colour = :red)
Error showing value of type Makie.FigureAxisPlot:
ERROR: MethodError: no method matching gl_convert(::Symbol)
Closest candidates are:
  gl_convert(::Observable{T}) where T<:GeometryBasics.Mesh at ~/.julia/packages/GLMakie/K6iJk/src/GLAbstraction/GLUniforms.jl:197
  gl_convert(::Observable{T}) where T<:Union{Float32, Float64, Int32, UInt32, GeometryBasics.ZeroIndex{Int32}, GLMakie.GLAbstraction.GLProgram, GLMakie.GLAbstraction.Shader, GLMakie.GLAbstraction.GPUArray, StaticArraysCore.SMatrix, StaticArraysCore.StaticArray{Tuple{N}, T, 1} where {N, T}, GeometryBasics.GLIndex} at ~/.julia/packages/GLMakie/K6iJk/src/GLAbstraction/GLUniforms.jl:227
  gl_convert(::Observable{T}) where T at ~/.julia/packages/GLMakie/K6iJk/src/GLAbstraction/GLUniforms.jl:228
  ...
Stacktrace:
  [1] #map#13
    @ ~/.julia/packages/Observables/PHGQ8/src/Observables.jl:564 [inlined]
  [2] map
    @ ~/.julia/packages/Observables/PHGQ8/src/Observables.jl:562 [inlined]
  [3] const_lift
    @ ~/.julia/packages/GLMakie/K6iJk/src/GLAbstraction/GLUtils.jl:107 [inlined]
  [4] gl_convert(s::Observable{Symbol})
    @ GLMakie.GLAbstraction ~/.julia/packages/GLMakie/K6iJk/src/GLAbstraction/GLUniforms.jl:228
  [5] GLMakie.GLAbstraction.RenderObject(data::Dict{Symbol, Any}, program::GLMakie.GLVisualizeShader, pre::GLMakie.GLAbstraction.StandardPrerender, post::Nothing, bbs::GeometryBasics.HyperRectangle{3, Float32}, main::Nothing)
    @ GLMakie.GLAbstraction ~/.julia/packages/GLMakie/K6iJk/src/GLAbstraction/GLTypes.jl:336
  [6] assemble_robj(data::Dict{Symbol, Any}, program::GLMakie.GLVisualizeShader, bb::GeometryBasics.HyperRectangle{3, Float32}, primitive::UInt32, pre_fun::Nothing, post_fun::Nothing)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/glshaders/visualize_interface.jl:100
  [7] assemble_shader(data::Dict{Symbol, Any})
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/glshaders/visualize_interface.jl:118
  [8] draw_lines(shader_cache::Any, position::Observable{Vector{Point{2, Float32}}}, data::Dict)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/glshaders/lines.jl:104
  [9] (::GLMakie.var"#195#196"{GLMakie.Screen, Scene, Lines{Tuple{Vector{Point{2, Float32}}}}})(gl_attributes::Dict{Symbol, Any})
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/drawing_primitives.jl:256
 [10] (::GLMakie.var"#173#176"{GLMakie.var"#195#196"{GLMakie.Screen, Scene, Lines{Tuple{Vector{Point{2, Float32}}}}}, GLMakie.Screen, Scene, Lines{Tuple{Vector{Point{2, Float32}}}}})()
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/drawing_primitives.jl:105
 [11] get!(default::GLMakie.var"#173#176"{GLMakie.var"#195#196"{GLMakie.Screen, Scene, Lines{Tuple{Vector{Point{2, Float32}}}}}, GLMakie.Screen, Scene, Lines{Tuple{Vector{Point{2, Float32}}}}}, h::Dict{UInt64, GLMakie.GLAbstraction.RenderObject}, key::UInt64)
    @ Base ./dict.jl:481
 [12] cached_robj!(robj_func::GLMakie.var"#195#196"{GLMakie.Screen, Scene, Lines{Tuple{Vector{Point{2, Float32}}}}}, screen::GLMakie.Screen, scene::Scene, x::Lines{Tuple{Vector{Point{2, Float32}}}})
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/drawing_primitives.jl:83
 [13] draw_atomic
    @ ~/.julia/packages/GLMakie/K6iJk/src/drawing_primitives.jl:242 [inlined]
 [14] insert!(screen::GLMakie.Screen, scene::Scene, x::Lines{Tuple{Vector{Point{2, Float32}}}})
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/drawing_primitives.jl:120
 [15] insertplots!(screen::GLMakie.Screen, scene::Scene)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/screen.jl:65
 [16] (::GLMakie.var"#104#106"{GLMakie.Screen})(s::Scene)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/screen.jl:67
 [17] foreach(f::GLMakie.var"#104#106"{GLMakie.Screen}, itr::Vector{Scene})
    @ Base ./abstractarray.jl:2774
 [18] insertplots!(screen::GLMakie.Screen, scene::Scene)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/screen.jl:67
 [19] (::GLMakie.var"#104#106"{GLMakie.Screen})(s::Scene)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/screen.jl:67
 [20] foreach(f::GLMakie.var"#104#106"{GLMakie.Screen}, itr::Vector{Scene})
    @ Base ./abstractarray.jl:2774
 [21] insertplots!(screen::GLMakie.Screen, scene::Scene)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/screen.jl:67
 [22] backend_display(screen::GLMakie.Screen, scene::Scene; connect::Bool)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/display.jl:20
 [23] backend_display(::GLMakie.GLBackend, scene::Scene; start_renderloop::Bool, visible::Bool, connect::Bool)
    @ GLMakie ~/.julia/packages/GLMakie/K6iJk/src/display.jl:5
 [24] backend_display
    @ ~/.julia/packages/GLMakie/K6iJk/src/display.jl:1 [inlined]
 [25] backend_display(s::Makie.FigureAxisPlot; kw::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Makie ~/.julia/packages/Makie/Ppzqh/src/display.jl:51
 [26] backend_display
    @ ~/.julia/packages/Makie/Ppzqh/src/display.jl:49 [inlined]
 [27] #display#932
    @ ~/.julia/packages/Makie/Ppzqh/src/display.jl:72 [inlined]
 [28] display(fig::Makie.FigureAxisPlot)
    @ Makie ~/.julia/packages/Makie/Ppzqh/src/display.jl:66
 [29] #invokelatest#2
    @ ./essentials.jl:729 [inlined]
 [30] invokelatest
    @ ./essentials.jl:726 [inlined]
 [31] print_response(errio::IO, response::Any, show_value::Bool, have_color::Bool, specialdisplay::Union{Nothing, AbstractDisplay})
    @ REPL ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/REPL.jl:296
 [32] (::REPL.var"#45#46"{REPL.LineEditREPL, Pair{Any, Bool}, Bool, Bool})(io::Any)
    @ REPL ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/REPL.jl:278
 [33] with_repl_linfo(f::Any, repl::REPL.LineEditREPL)
    @ REPL ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/REPL.jl:521
 [34] print_response(repl::REPL.AbstractREPL, response::Any, show_value::Bool, have_color::Bool)
    @ REPL ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/REPL.jl:276
 [35] (::REPL.var"#do_respond#66"{Bool, Bool, REPL.var"#77#87"{REPL.LineEditREPL, REPL.REPLHistoryProvider}, REPL.LineEditREPL, REPL.LineEdit.Prompt})(s::REPL.LineEdit.MIState, buf::Any, ok::Bool)
    @ REPL ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/REPL.jl:857
 [36] #invokelatest#2
    @ ./essentials.jl:729 [inlined]
 [37] invokelatest
    @ ./essentials.jl:726 [inlined]
 [38] run_interface(terminal::REPL.Terminals.TextTerminal, m::REPL.LineEdit.ModalInterface, s::REPL.LineEdit.MIState)
    @ REPL.LineEdit ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/LineEdit.jl:2510
 [39] run_frontend(repl::REPL.LineEditREPL, backend::REPL.REPLBackendRef)
    @ REPL ~/.julia/juliaup/julia-1.8.3+0.x64.linux.gnu/share/julia/stdlib/v1.8/REPL/src/REPL.jl:1248
 [40] (::REPL.var"#49#54"{REPL.LineEditREPL, REPL.REPLBackendRef})()
    @ REPL ./task.jl:484

I mean, it would be relatively easy to just check the user-provided kwargs at the beginning of the recipe. If any of the kwargs are not in our existing list of attributes, we just throw a warning. I would be a bit hesitant to let the recipe throw an error because maybe users actually want to store some additional attributes inside the plot object (and who am I to judge them).

Still, both ways to do this have their ups and downs. Would you be happy with such a warning being printed to the REPL when there's an unsupported keyword argument like agents_step!? Or would you rather not have the recipe do anything at all?

[fbanning]

P.s.: Oh, and that it's even possible to add arbitrary keyword arguments is due to Makie's recipe system afaict.

[Datseris]

I would be a bit hesitant to let the recipe throw an error because maybe users actually want to store some additional attributes inside the plot object (and who am I to judge them).

But it isn't the users that decide the API. We are. The users decide how to use the API, but they don't construct it. From our perspective, there is no reason to allow users to use the plot object to store additional stuff. They should use their own containers, or the properties container of the ABM. So, there should be a direct error if a keyword not in the recipe attributes is provided. What I am worried about is that this may be a pain in the ass to maintain. Like, if we add more features to the recipe, we would also have to hunt around all the error checking code as well.

Maybe best to fix this unecessary problem at the source. Why would a recipe by construction allow any possible keyword to be passed as valid? This doesn't make sense. cc @SimonDanisch ?

[iago-lito]

As a user, I agree that I would be very okay to use my own data structures to hold additional data, and have the API hold me tight to prevent this kind of errors because they are meaningless yet with a lot of consequences and yet again easy to spot for machines.

maybe users actually want to store some additional attributes inside the plot object

I am not aware of such a practice. But if it's common, then maybe one correct solution is to provide one user-dedicated additional_data::Any=nothing keyword so they can input anything they want inside? Otherwise, if they start using any keyword unused/unchecked yet by the API, then any future keyword you would like to add to your official signature (or "more features to the recipe" as you write) would become breaking because it could potentially clash with theirs, which sounds very bad IMO.

[fbanning]

Sure, I get your points.

What I am worried about is that this may be a pain in the ass to maintain. Like, if we add more features to the recipe, we would also have to hunt around all the error checking code as well.

Error hunting wouldn't be required if we choose the path I described above, or am I mistaken?

just check the user-provided kwargs at the beginning of the recipe. If any of the kwargs are not in our existing list of attributes, we just throw a warning.

Of course we would then throw an error instead of a warning.

Let's wait if @SimonDanisch can shed some light on how unsupported attributes in recipes are handled by Makie and what could be done to prevent the issue described here. Maybe I misunderstood how the recipe system works after all and there's a specific route that I'd have to take to also get the warning that Makie throws on unsupported kwargs in built-in recipes.

[SimonDanisch]

Nah, this is a long standing issue...
It's a bit historical and unfortunate, since in the first prototype for Makie I had pretty strict attribute checking and nice errors for wrong attributes - which I had to throw out in a big refactor and never made it back.
I need to find some time to bring this back, I think it's a really important feature.

[fbanning]

Awesome, thanks for the info.

Maybe it's best for us not to hack anything together until this is fixed at its core? If the problem pops up a few more times in the meantime, we can still set aside a bit of time to temporarily fix it then.

[Datseris]

I agree! We keep this open and maybe someone with similar problem will see it.

[fbanning]

I've also added the documentation label. In the PR following #914 I would like to include a short notice in the docs that tell people about this behaviour in the recipe system. After that has been done, we can close this issue.