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

Use NoSpaceAgent alongside space agents #719

Open
veddox opened this issue Dec 20, 2022 · 8 comments · May be fixed by #720
Open

Use NoSpaceAgent alongside space agents #719

veddox opened this issue Dec 20, 2022 · 8 comments · May be fixed by #720

Comments

@veddox
Copy link
Contributor

veddox commented Dec 20, 2022

Is your feature request related to a problem? Please describe.
I have a model with multiple agent types, some of which require (grid-)space, and others for which a position argument doesn't really make sense. I would like to be able to use NoSpaceAgent as the base type for the latter, but this causes agent_validator() and do_checks() to throw an ArgumentError: "Second field of Agent struct must be pos when using a space."

Describe the solution you'd like
I'm wondering whether agent_validator() is still necessary, since creating one's own agents structs has been deprecated? It also gives me an annoying warning about a non-concrete Agent type, as I use a Union type for my multi-agent model.

Describe alternatives you've considered
If it's important to keep agent_validator(), it would be nice to include a check for whether the checked agent type extends NoSpaceAgent, and if so, exclude it from the spatial checks. However, I'm not sure how to do this, as agent types created with @agent only have AbstractAgent as a supertype, even if created with NoSpaceAgent as a base type.

@Datseris
Copy link
Member

Why is passing warn=false when making the ABM not an option...?

@veddox
Copy link
Contributor Author

veddox commented Dec 20, 2022

First, thanks for pointing out warn, I had overlooked that so far. However, it doesn't solve the problem, as it only turns off some of the checks (those for type concreteness, mutability, and velocity type). The check that's bringing me up short (:pos as second argument) is not affected by it.

I appreciate the defensive programming on display with these functions, but like I said I'm not sure they're still necessary, at least not in that form? Also, the use of warn seems a bit inconsistent.

Lastly, I think my situation (needing NoSpaceAgent as well as GridAgent) is well within the realms of the expectable, and should be provided for without having to turn off default behaviour?

@veddox
Copy link
Contributor Author

veddox commented Dec 20, 2022

function do_checks(::Type{A}, space::S, warn::Bool) where {A<:AbstractAgent,S<:SpaceType}
    if warn
        isbitstype(A) &&
        @warn "AgentType is not mutable. You probably haven't used `@agent`!"
    end
    (any(isequal(:id), fieldnames(A)) && fieldnames(A)[1] == :id) ||
    throw(ArgumentError("First field of Agent struct must be `id` (it should be of type `Int`)."))
    fieldtype(A, :id) <: Integer ||
    throw(ArgumentError("`id` field in Agent struct must be of type `Int`."))
    if space !== nothing
        space_type = typeof(space)
        if warn && !(any(isequal(:pos), fieldnames(A)) && fieldnames(A)[2] == :pos)
            @warn "Second field of Agent struct should be `pos` when using a space."
        elseif any(isequal(:pos), fieldnames(A))
            # Check `pos` field in A has the correct type
            pos_type = fieldtype(A, :pos)
            if space_type <: GraphSpace && !(pos_type <: Integer)
                throw(ArgumentError("`pos` field in Agent struct must be of type `Int` when using GraphSpace."))
            elseif space_type <: GridSpace && !(pos_type <: NTuple{D,Integer} where {D})
                throw(ArgumentError("`pos` field in Agent struct must be of type `NTuple{Int}` when using GridSpace."))
            elseif space_type <: ContinuousSpace || space_type <: ContinuousSpace
                if !(pos_type <: NTuple{D,<:AbstractFloat} where {D})
                    throw(ArgumentError("`pos` field in Agent struct must be of type `NTuple{<:AbstractFloat}` when using ContinuousSpace."))
                end
            end
        end
        if warn && space_type <: ContinuousSpace &&
            any(isequal(:vel), fieldnames(A)) &&
            !(fieldtype(A, :vel) <: NTuple{D,<:AbstractFloat} where {D})
            @warn "`vel` field in Agent struct should be of type `NTuple{<:AbstractFloat}` when using ContinuousSpace."
        end
    end
end

My suggestion would be this. This converts the ArgumentError about the missing :pos into a @warn (which can be silenced), and only carries out the :pos-related checks if we actually have a spatial agent type.

(Note that there is a line near the bottom with a duplicate check for space_type <: ContinuousSpace. I'm not sure whether the second check is supposed to check for a different space type, or whether it's superfluous?)

What do you think?

@Datseris
Copy link
Member

yes, I agree. (You'll open a PR I assume)

@veddox
Copy link
Contributor Author

veddox commented Dec 21, 2022

Working on it 👍

Could you give me a quick feedback on the duplicate ContinuousSpace check above? (Here in the current code.) This seems like something that should be fixed, but I'm not sure what the original intention was.

@Datseris
Copy link
Member

oh that's just a mistake

@PiotrZakrzewski
Copy link

The proposed solution of adding the NoSpaceAgents to a special pos e.g. 1,1 causes a following problen: If you use add_agent! it checks for space being occupied. Is there a way to circumvent it?

@Datseris
Copy link
Member

What problems would you encounter if you simply had a vector of these extra "no-space-agents" as a model property? The normal model agents are the ones living in space, and you have another agent container in one of the model properties.

What problems would this make for you?

We still haven't resolved how to handle "no space agents" and "normal agents" together in one model, primarily because so far I haven't seen a concrete problem with just making these extra agents a model property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants