warn if placing already placed agent

[puer-robustus]

While it might be desired in a specific model to have the same agent be placed in multiple spots simultaneously, the typical use case is that one agent has one position at every given moment. This commit adapts the place_agent() method in a way that it emits a warning when called with an agent which already has a location.

Fixes: #1522

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔵 +0.9% [+0.7%, +1.1%]	🔵 +2.6% [+2.4%, +2.9%]
Schelling	large	🔵 +1.6% [+0.6%, +2.5%]	🔴 +16.7% [+14.8%, +18.2%]
WolfSheep	small	🔵 +0.6% [-1.0%, +2.3%]	🔵 +1.1% [+0.9%, +1.3%]
WolfSheep	large	🔵 +0.4% [-0.4%, +1.3%]	🔴 +7.2% [+5.0%, +10.5%]
BoidFlockers	small	🔴 +1453.8% [+1412.0%, +1498.5%]	🔴 +3.9% [+3.1%, +4.8%]
BoidFlockers	large	🔴 +1447.2% [+1389.3%, +1515.0%]	🔵 +2.5% [+1.8%, +3.1%]

[puer-robustus]

Hi all,

here goes my first PR to Mesa :)

Few comments on the code on which I'd appreciate your feedback:

1. From reading the discussion in Grid: Calling place_agent twice in different positions causes an agent to exist in two positions in the grid #1522, I felt like adding a warning via a decorator would be a fairly simple thing to do. In its current form, however, it is very tightly coupled to the place_agent() method (in naming at least). I'm wondering whether there are useful generalization possibilities - but can't tell because I am fairly new to Project Mesa. Feel free to hint me in a particular direction on which other methods could use similar checks.
2. I could not find an existing logging configuration in the project. Did I miss it somehow?

Thanks!

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔵 -0.8% [-1.3%, -0.4%]	🔵 -1.3% [-1.5%, -1.1%]
Schelling	large	🔵 -0.4% [-1.0%, +0.1%]	🟢 -7.9% [-9.8%, -5.8%]
WolfSheep	small	🔵 +0.0% [-0.6%, +0.6%]	🔵 +0.4% [+0.3%, +0.5%]
WolfSheep	large	🔵 +0.1% [-0.5%, +0.6%]	🔵 +0.7% [-0.7%, +2.4%]
BoidFlockers	small	🔴 +1458.5% [+1417.3%, +1501.5%]	🔵 +1.2% [+0.3%, +2.1%]
BoidFlockers	large	🔴 +1514.9% [+1450.6%, +1580.8%]	🔵 +1.2% [+0.7%, +1.7%]

[quaquel]

I could not find an existing logging configuration in the project. Did I miss it somehow?

There currently is no logging configuration. It is very much another item on the todo list.

[rht]

I tested and it works:

WARNING:You're placing agent 2699 which already has
the position (9, 1). In most cases, you'd want to clear the current
position with remove_agent() before placing the agent again.

Though the warning message should have been made clearer to say that it is from a place_agent call, otherwise I wouldn't know the source line that causes the warning.

[EwoutH]

Thanks for this PR, it would be a nice feature to have.

For everyone, one thing I'm not sure about if we should make this warning optional or not.

[puer-robustus]

There currently is no logging configuration. It is very much another item on the todo list.

For everyone, one thing I'm not sure about if we should make this warning optional or not.

Maybe it is preferable if I stick with warnings.warn() for the time being?
For one thing, I am not keen on introducing some half-baked logging approach which might not fit the needs of the project. Secondly, while one could make the default logging.Level configurable in many ways, suppressing warnings from warnings is accomplished fairly simply by running python with the -W flag.

Though the warning message should have been made clearer to say that it is from a place_agent call, otherwise I wouldn't know the source line that causes the warning.

I see the point. How far should I go: Only adapt the warning message that the root cause is a place_agent() call or actually show some kind of "stack trace" with the exact location of the "dangerous" invocation?

[EwoutH]

Maybe it is preferable if I stick with warnings.warn() for the time being?

Yes, I think that would be fine and simplify this PR.

Only adapt the warning message that the root cause is a place_agent() call

I think this is good enough.

[puer-robustus]

So, warnings.warn() with the stacklevel=2 parameter actually gives a bit of an indication on the problematic call:

/tmp/ipykernel_11991/541206631.py:5: UserWarning: Agent 0 is being placed with
place_agent() despite already having the position (1, 1). In most
cases, you'd want to clear the current position with remove_agent()
before placing the agent again.
  grid.place_agent(a, (0, 0))

Hope that's ok with you.

[rht]

So, warnings.warn() with the stacklevel=2 parameter actually gives a bit of an indication on the problematic call:

Yes, that's sufficiently informative.

---

Meta: I support your GitHub protest. It's just that there is no bridge to other ethical FOSS tools yet to ensure public record of discussions and to remain inclusive to people who still use GitHub to contribute to Mesa. Maybe there will be such bridge (along the line of the Matrix bridges that have been built with Slack, Discord, etc).

[puer-robustus]

Despite being off-topic:

Meta: I support your GitHub protest.

Thanks for the support. Much appreciated. Cause sometimes it feels like I'm fighting windmills.

Maybe there will be such bridge (along the line of the Matrix bridges that have been built with Slack, Discord, etc).

The folks over at Forgejo are trying to build federation for git forges based on the ActivityPub protocol. I doubt GitHub will ever federate though :/ And Drew DeVault's reservations on the ActivityPub integration for git still hold.

[EwoutH]

Hmmm we took quite a big hit on initialization time here. Maybe we should reconsider this approach.

[puer-robustus]

Since there are no expensive operations in the decorator, except for writing warnings to stdout, I think this hints at a "problem" in the benchmark file:

            boid = Boid(
                unique_id=i,
                model=self,
                pos=pos,
                speed=self.speed,
                direction=direction,
                vision=self.vision,
                separation=self.separation,
                **self.factors,
            )
            self.space.place_agent(boid, pos)

This looks exactly like the pattern the warning wants to hint at: An agent having a position and being assigned another one before getting that attribute cleared. Not sure whether this is intended in this case.

[rht]

I fixed the benchmarks performance problem: #2086 (comment)