space: Confusing API with methods with similar names

[rht]

The class Grid has:

• neighbor_iter: "Iterate over position neighbors"
• iter_neighborhood: "Return an iterator over cell coordinates that are in the neighborhood of a certain point"
• iter_neighbors: "Return an iterator over neighbors to a certain point"

neighbor_iter and iter_neighbors seem to be a duplicate of each other.
iter_neighborhood should probably be called iter_neighbor_coordinates due to the type of its return value.

[Corvince]

neighbor_iter and iter_neighbors seem to be a duplicate of each other.

Yes, except neighbor_iter has a restricted API, but HexGrid only implements neighbor_iter and not iter_neighbors 🤦

iter_neighbors should be the only function IMO.

iter_neighborhood should probably be called iter_neighbor_coordinates due to the type of its return value.

Disagree. For me it was always clear that neighbors => Agents, neighborhood => Coordinates

[rht]

Disagree. For me it was always clear that neighbors => Agents, neighborhood => Coordinates

Is it just me or that the dictionary definition of "neighborhood" refers to people, not geographical coordinates?

From https://en.wikipedia.org/wiki/Neighbourhood:

A neighbourhood (British English, Australian English and Canadian English) or neighborhood 
(American English; see spelling differences—u is omitted in American English) is a geographically 
localised community within a larger city, town, suburb or rural area. Neighbourhoods are often 
social communities with considerable face-to-face interaction among members.

[Corvince]

Is it just me or that the dictionary definition of "neighborhood" refers to people, not geographical coordinates?

Well maybe look it up in a dictionary!?

From https://www.dictionary.com/browse/neighborhood?s=t

the area or region around or near some place or thing; vicinity

If you would have read the wikipedia just a little bit further, it would also say this:

Neighbourhood is generally defined spatially as a specific geographic area and functionally as a set of social networks.

So both definition exists and I definitely see your point, but actually both definitions are irrelevant in this context.

The origin of neighborhoods in the context of ABMs relies on the mathematical definition of neighborhoods, which is just an open set of points that contains a specific point. No people involved

[rht]

If iter_neighborhood is find, then the problematic wording is the neighbor_iter and iter_neighbors. They should be called iter_neighborhood_content for clarity.

[rht]

Since a good method name pattern is something along the line of "verb_object", iter_neighbors sounds better for me than neighbor_iter. But iter_neighbors is never used in the examples.

[dmasad]

My suggestion for an alternative API is to make it explicit what's being returned. So get_neighbors would become get_neighboring_agents and get_neighborhood would become get_neighboring_coordinates. We could keep the old signatures as well for a while, and have them be pass-throughs to the new methods along with a DeprecationWarning.

[jackiekazil]

RE: #1184 - this I was looking it over. I really appreciate the clean up. @rht is there a reason why you chose iter_neighbors over get_neighbors? The reason I ask is because to newer users, get_neighbors is easier to understand.

[tpike3]

RE: #1184 - this I was looking it over. I really appreciate the clean up. @rht is there a reason why you chose iter_neighbors over get_neighbors? The reason I ask is because to newer users, get_neighbors is easier to understand.

There is a subtle distinction here as get_neighbors is a function as well:

get_neighbors: Returns the objects surrounding a given cell.
get_neighborhood: Returns the cells surrounding a given cell.
iter_neighbors: Iterates over position neighbors.
iter_neighborhood: Returns an iterator over cell coordinates that are in the neighborhood of a certain point.

I concur new users will use get_neighbors as more intuitive but as they get more sophisticated iter_neighbors will provide more flexibility with an iterator.

[rht]

I didn't "choose" iter_neighbors in #1184, since both iter_neighbors and get_neighbors are already present. That PR specifically deletes only neighbor_iter.

The further deletion of iter_* in favor of get_* can be done in a separate PR. Regarding with returning iterator being confusing, I think it is fine, since people are already used to range(5) being an iterator anyway.

[jackiekazil]

The pr was merged. Ty.

[rht]

#815 (comment) contains argument on why it is beneficial to use get_* over iter_*: a list can be cached, but an iterator cant. With this, I'm convinced that get_* that returns list is the way to go.

[rht]

On reviewing #815, I realized that the caching mechanism of neighborhood has already been incorporated in current main. This means that both iter_neighbors and get_neighbors use cached result of the neighborhood. OK, this means that it is still performant to use only get_neighbors that returns an iterator.