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

Deprecate initialize_data_collector #2327

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Sep 25, 2024

Deprecate the Model's initialize_data_collector, which was added in #1287 to solve #1221.

Back when we had a schedulers it was logical, but now you can't forget to add Agents to a scheduler since that happens automatically.

The other reason was the batch_run assuming a Model attribute called datacollector to be present. An error in batch_run itself is far more robust to enforce that behavior, so that's added..

It also removes the behavior of directly collecting after init. Now you can do it when you want. It makes using the DataCollector explicit.

The migration guide was updated. Basically, replace:

self.initialize_data_collector(...)

With:

self.datacollector = DataCollector(...)

@EwoutH EwoutH added backport-candidate PRs we might want to backport to an earlier branch deprecation When a new deprecation is introduced labels Sep 25, 2024
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔴 +5.2% [+4.3%, +6.1%] 🔵 -0.1% [-0.3%, +0.0%]
BoltzmannWealth large 🔵 +0.2% [-0.2%, +0.6%] 🔴 +4.6% [+3.4%, +6.0%]
Schelling small 🔵 +0.3% [-0.1%, +0.7%] 🔵 +0.1% [-0.1%, +0.3%]
Schelling large 🔵 +2.2% [+1.1%, +3.2%] 🔵 +2.2% [-0.4%, +4.7%]
WolfSheep small 🔵 +1.5% [+1.3%, +1.7%] 🔵 +0.8% [+0.6%, +1.0%]
WolfSheep large 🔵 +0.4% [-0.9%, +1.6%] 🔵 +0.3% [-4.2%, +4.6%]
BoidFlockers small 🔵 +2.8% [+2.4%, +3.2%] 🔵 +1.6% [+1.0%, +2.3%]
BoidFlockers large 🔵 +3.3% [+2.5%, +4.1%] 🔵 +2.2% [+1.6%, +2.7%]

@EwoutH EwoutH requested review from rht, Corvince and tpike3 September 25, 2024 16:11
@rht
Copy link
Contributor

rht commented Sep 25, 2024

The other reason was the batch_run assuming a Model attribute called datacollector to be present. An error in batch_run itself is far more robust to enforce that behavior, so that's added..

Another reason is that there could be multiple data collector objects. As of today, I don't think there are any reasonable use cases that necessitate multiple data collectors (each with own scheduler/agent containers) anymore.

I'm fine with reducing the API surface, i.e. deleting this method.

@EwoutH EwoutH merged commit 28e52d5 into projectmesa:main Sep 26, 2024
10 of 12 checks passed
EwoutH added a commit that referenced this pull request Sep 26, 2024
Deprecate the Model's `initialize_data_collector`.

Back when we had a schedulers it was logical, but now you can't forget to add Agents to a scheduler since that happens automatically.

The other reason was the `batch_run` assuming a Model attribute called `datacollector` to be present. An error in `batch_run` itself is far more robust to enforce that behavior, so that's added..

It also removes the behavior of directly collecting after init. Now you can do it when you want. It makes using the DataCollector explicit.

The migration guide was updated. Basically, replace:
```python
self.initialize_data_collector(...)
```

With:
```python
self.datacollector = DataCollector(...)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate PRs we might want to backport to an earlier branch deprecation When a new deprecation is introduced
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants