JuliaDynamics / Agents.jl

Agent-based modeling framework in Julia
https://juliadynamics.github.io/Agents.jl/stable/
MIT License
760 stars 123 forks source link

API Clarifications #301

Closed Datseris closed 4 years ago

Datseris commented 4 years ago

I think the names we have chosen for all these node_neighbors and space_neighgbhors are not optimal and can be confusing. I'd suggest the alternatives:

or even better, to save space and not having to type one of the most difficult words in the english lexicon:

which can deprecate the existing names.

Datseris commented 4 years ago

Further suggestioN: rename nodes function into positions that iterates over the "positions" of the space, which are either Int or Tuple.

Datseris commented 4 years ago

Also: get_node_contents -> agents_in_pos

Libbum commented 4 years ago

I'd agree with all these changes. We'd need depreciation guards. Most importantly, this would bump us to 4.0 yes? It's a breaking change.

Datseris commented 4 years ago

Not with deprecations. You can deprecate @deprecate get_node_contents agents_in_pos and nothing breaks. However depending on the changes to space we might go into a new version. I am working on the new space now...

Libbum commented 4 years ago

Let's do this once #295 is merged, provided @kavir1698 is on board.

kavir1698 commented 4 years ago

Yeah, I agree with the changes. The new names are easier to remember.

fbanning commented 4 years ago

If I may add something to the discussion: I found these names to be confusing as well. nearby_agents would intuitively make a lot more sense. I would argue that nearby_positions would not be in line with the standard naming for the positional agent field pos and therefore nearby_pos could make more sense here.

Datseris commented 4 years ago

Good point but there is another difficulty: nearby_pos hints at singular, and I would imagine it would return a single, random, nearby position.

fbanning commented 4 years ago

Maybe it might also be interesting to have a look at how other APIs did it. Not saying that how others did it would be the best approach, but maybe it might help with transitions (no need to "reinvent the wheel" when it comes to function naming).

For example NetLogo (which is widely used in the ABM community as you all might know) has neighbors (moore) and neighbors4 (von Neumann). See their documentation for this. They both return a list of neighboring patches (which equals to a list of pos in our case).

Of course we don't need to differentiate between Moore and von Neumann, so it will be a bit simpler in our case. Following this, we could also name our function simply neighbors(x, model; include_self = false) which might return either

So if you search neighbors of an agent, you get a list of agents back. And if you search neighbors of a coordinate/position, you get a list of positions back. The new include_self keyword defaults to false so that normally you only collect surrounding agents/positions which I expect to be the standard case. Overall such an approach would intuitively make more sense to me but I might be primed by my previous experience with other languages.

This would be a more fundamental API change of which I'm not sure if it's wanted but I at least wanted to bring it up in this discussion. :)

Datseris commented 4 years ago

Another important point: decide ONE terminology between CELL, NODE and VERTEX and use only this one throughout the source. I mean, damn, we use all three words interchangeably!

Datseris commented 4 years ago

I disagree with the include_self keyword which I think shouldn't exist because the agent itself is not its neighbor by definition.

fbanning commented 4 years ago

I disagree with the include_self keyword which I think shouldn't exist because the agent itself is not its neighbor by definition.

I actually agree with you and only included the keyword for feature parity with the current API.

Datseris commented 4 years ago

I think after consideration I'd vote for the syntax nearby_agents and nearby_positions but both excluding the current position or current agent that they are called with.

fbanning commented 4 years ago

decide ONE terminology between CELL, NODE and VERTEX

I think "node" might be the most widely applicable name. "Cell" invokes a direct relation to cellular automata and "vertex" to graphs - both do not seem general enough to me.

kavir1698 commented 4 years ago

I vote neighbors. It is simpler. I'd also excluding focal agent/position. Node is better than vertex because vertex is more used in graphs theory, and node is more common in network science.

Datseris commented 4 years ago

Notice however that neighbors as a name conflicts with LightGraphs.neighbors, and I find it likely that a user of Agents would use LightGraphs as well in parallel.

kavir1698 commented 4 years ago

Oh yeah, then it's not a good name.

Libbum commented 4 years ago

Gridcell and graph node. But I'd also vote for node too: It's understandable enough in a grid context, and also is akin to point in continuous space (where cell is more of a box in that context).

fbanning commented 4 years ago

Hm, that name conflict with LightGraphs is indeed a pity. I would really like for the Agents API to be as self-explanatory and simple as possible. I don't know how to solve this name conflict though. :/

Also making use of multiple dispatch makes sense to me as it further simplifies the API. I'm not a big fan of having an own function name for agents and nodes although I definitely think that the nearby_ names are a lot nicer than what we currently have! :)

Datseris commented 4 years ago

I don't think the separation in nearby_positions and nearby_agents is bad, and here is why: They are typically called with different applications in mind.

nearby_agents is used when you want to do interaction and exchange of information between agents.

nearby_positions is called when you want to move an agent.

EDIT: One more thing to consider: in the "nearest neighbors" community and all neighbor finding functions, the function called neigbhors is always returning the underlying data points. For us the data points are agents. It is a-typical for a function called neighbors to return spatial information (unless of course, the data point and the spatial information coincide).

Libbum commented 4 years ago

Anyone else (users included!) want to weigh in on this, feel free.

Seems like the consensus at the moment is

nearby_agents, nearby_positions and node.

Datseris commented 4 years ago

Alright https://github.com/JuliaDynamics/Agents.jl/pull/304 was pretty big. We should now put these suggestions into place as well. Most importantly, nearby_agents must become the 5th necessary function one needs to implement for a new space type, and should be put in abstract terms under https://github.com/JuliaDynamics/Agents.jl/blob/master/src/core/space_interaction_API.jl#L20 .

nearby_positions is only valid for discrete spaces and thus should not be part of the mandatory API.

Datseris commented 4 years ago

Once nearby_agents is there, we should write a "Developer's docs" page.

Datseris commented 4 years ago

One more good argument for separating nearby_positions and nearby_agents @fbanning : At several points you might want to do the operation nearby_agents(pos, model) because you want e.g. "all agents nearby the town square". With your suggestion of neighbors dispatching on the first argument, this is not possible.

Datseris commented 4 years ago

while we're at it, get_node_contents -> node_contents, find_empty_nodes -> empty_nodes.

Libbum commented 4 years ago

Is this something to move on now, or should I wait for the Grid->Array change?

Datseris commented 4 years ago

definitely wait :P

Datseris commented 4 years ago

Just to clarify: @kavir1698 , @Libbum , why exactly do we need the word "node"? As far as I can tell, all "node" and "position" are the same thing (for discrete spaces, where the concept of "node" is valid). So why use both "node" and "position", when one can use just "position"?

EDIT: Okay I think I see the point: "node" refers an entity in the space containing the agents, while "position" refers to its exact position in space? So that we can say something like: " Return a random position of an empty node " ? Still not sure though :D

EDIT2: Actually, we could say: " Return a random position without any agents, or nothing if no such positions exist. ". So I still see the possibility of also completely removing the word "nodes"...?

Datseris commented 4 years ago

Also: pick_empty -> random_empty.

Libbum commented 4 years ago

Sure! position is certainly more space agnostic than node. It's not a precise word, but I think that's a decent trade-off when we have multiple subtypes like this.

Datseris commented 4 years ago

Also, be consistent with multi agent models or mixed agent models... How do we want to call these? Let's just pick one name and stick with it.

kavir1698 commented 4 years ago

I am ok with sticking to "position".

"Multi-agent model" is a common synonym for ABM. Mixed-agent models is better.

Datseris commented 4 years ago

ALRIGHTY THEN. Let's collect the changes:

  1. space_neighbors -> nearby_agents
  2. node_neighbors -> nearby_positions
  3. get_node_contents -> agents_in_pos
  4. Multi agent model -> mixed agent model (if it is somewhere)
  5. pick_empty -> random_empty
  6. find_empty_nodes -> empty_nodes
  7. Replace all instances of the word "node" with the word "position" (and also nodes with positions). This will change both wording in documentation string but also function names, e.g. nodes -> positions.

Anyone up in doing this monumental change carefully, deprecating all changed names with @deprecate oldname newname and adding the changes in the changelog?

Libbum commented 4 years ago

I can do it.