Skip to content

datacollector: store separate snapshots of model data per step #2129

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
May 8, 2024

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented May 6, 2024

This pull request addresses issue #2128, which involves incorrect data collection for PropertyLayer objects when using Mesa's DataCollector module. Previously, the DataCollector was maintaining references to the PropertyLayer data array, leading to every step showing identical data due to changes propagating backward.

Changes Introduced:

  • Updated the collect method of the DataCollector to use deepcopy for model-level data collection.
  • Ensures that unique snapshots of data are collected at each model step rather than maintaining references.

Resolves #2128.

- Ensure `DataCollector` stores unique copies of the data at each model step.
- Use `deepcopy` in `collect` method to avoid storing references that lead to each step holding the same data.
- Fixes the issue where PropertyLayer data was stored identically for all steps, preventing accurate visualization and analysis of model progression.
@EwoutH EwoutH requested review from rht, quaquel and tpike3 May 6, 2024 09:15
Copy link

github-actions bot commented May 6, 2024

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 +0.1% [-0.4%, +0.6%] 🔵 -0.5% [-0.9%, -0.2%]
Schelling large 🔵 +0.2% [-0.9%, +1.2%] 🔵 +2.6% [-2.3%, +6.6%]
WolfSheep small 🔵 +1.3% [-0.1%, +2.9%] 🔵 -0.1% [-0.5%, +0.3%]
WolfSheep large 🔵 +0.5% [+0.1%, +0.9%] 🔵 +0.2% [-0.9%, +1.1%]
BoidFlockers small 🔵 +0.3% [-0.3%, +0.9%] 🔵 +0.4% [-0.5%, +1.2%]
BoidFlockers large 🔵 -0.5% [-1.2%, +0.2%] 🔵 +0.1% [-0.8%, +0.9%]

@EwoutH
Copy link
Member Author

EwoutH commented May 6, 2024

@dylan-munson this PR should resolve your issue (I tested it locally and for me it does). You can install this branch with:

pip install -U -e git+https://github.com/projectmesa/mesa@datacollector_deepcopy#egg=mesa

Thanks for reporting it and please let us know if this fixed it for you.

@EwoutH EwoutH changed the title datecollector: store separate snapshots of model data per step datacollector: store separate snapshots of model data per step May 6, 2024
@EwoutH EwoutH added the bug Release notes label label May 6, 2024
@dylan-munson
Copy link

@EwoutH Thanks for looking into this; I will try this in a bit. Just to check, I will be able to return to the main branch of the mesa repo by just running a usual pip install update right?

@dylan-munson
Copy link

I can confirm that after installing this dev branch the issue has been fixed. Thank you for the help! I'll keep my eyes open for when this makes it into the main branch.

@@ -197,17 +198,19 @@ def collect(self, model):
for var, reporter in self.model_reporters.items():
# Check if lambda or partial function
if isinstance(reporter, (types.LambdaType, partial)):
self.model_vars[var].append(reporter(model))
self.model_vars[var].append(deepcopy(reporter(model)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a brief explanatory comment before this line. Otherwise someone might mistakenly remove the deepcopy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done.

@EwoutH
Copy link
Member Author

EwoutH commented May 8, 2024

@dylan-munson Thanks for validating that this fix resolves your issue!

@rht Thanks for reviewing. I added the comment and resolved the merge conflict. Please squash when merging.

@rht
Copy link
Contributor

rht commented May 8, 2024

Can't merge due to conflicts.

@rht rht merged commit abac825 into main May 8, 2024
3 of 9 checks passed
@rht rht deleted the datacollector_deepcopy branch May 8, 2024 06:57
@rht
Copy link
Contributor

rht commented May 8, 2024

Squash and merge worked.

EwoutH added a commit that referenced this pull request May 8, 2024
* datecollector: store separate snapshots of model data per step
- Ensure `DataCollector` stores unique copies of the data at each model step.
- Use `deepcopy` in `collect` method to avoid storing references that lead to each step holding the same data.
- Fixes the issue where PropertyLayer data was stored identically for all steps, preventing accurate visualization and analysis of model progression.

* datacollection: Add deepcopy comment
@EwoutH
Copy link
Member Author

EwoutH commented Jul 3, 2024

@dylan-munson I cherry picked this PR to the 2.3 maintenance branch, and tagged Mesa 2.3.1 which includes this fix. So you can now also use that version.

vitorfrois pushed a commit to vitorfrois/mesa that referenced this pull request Jul 15, 2024
…ctmesa#2129)

* datecollector: store separate snapshots of model data per step
- Ensure `DataCollector` stores unique copies of the data at each model step.
- Use `deepcopy` in `collect` method to avoid storing references that lead to each step holding the same data.
- Fixes the issue where PropertyLayer data was stored identically for all steps, preventing accurate visualization and analysis of model progression.

* datacollection: Add deepcopy comment
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with data collection of PropertyLayer objects
3 participants