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

added empty iterable checks and updated tests #2523

Merged
merged 2 commits into from
Dec 1, 2024

Conversation

Sahil-Chhoker
Copy link
Contributor

Summary

Fixed the handling of empty iterables in _make_model_kwargs. Now raises a ValueError when an empty list is passed.

Bug / Issue

Closes #2108.
Empty iterables weren't properly handled.

Implementation

Updated _make_model_kwargs to raise a ValueError for empty iterables.
added a new function test_batch_run_with_params_with_empty_content() for testing.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
BoltzmannWealth small 🔵 +4.5% [+2.8%, +6.1%] 🔵 +0.3% [+0.1%, +0.5%]
BoltzmannWealth large 🔵 +0.2% [-0.5%, +1.0%] 🔵 +3.2% [+1.1%, +4.9%]
Schelling small 🔵 +0.9% [+0.6%, +1.1%] 🔵 +0.7% [+0.4%, +0.9%]
Schelling large 🔵 +0.6% [+0.2%, +0.9%] 🔵 +5.0% [+2.5%, +7.2%]
WolfSheep small 🔵 +1.8% [+1.5%, +2.2%] 🔵 +0.7% [+0.4%, +1.0%]
WolfSheep large 🔵 +2.4% [+1.0%, +4.0%] 🔴 +10.7% [+5.8%, +15.9%]
BoidFlockers small 🔵 -0.5% [-1.1%, +0.1%] 🔵 -0.3% [-1.3%, +0.5%]
BoidFlockers large 🔵 -0.6% [-1.1%, +0.1%] 🔵 -0.7% [-1.1%, -0.3%]

@Sahil-Chhoker
Copy link
Contributor Author

Hey @quaquel can you please review the changes for this pull request.

@quaquel
Copy link
Member

quaquel commented Dec 1, 2024

This is a part of the code base that I am not intimately familiar with, so I need some more time. I hope to get to it somewhere in the coming days.

Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, this is indeed an useful edge-case to catch.

@EwoutH EwoutH added the maintenance Release notes label label Dec 1, 2024
@EwoutH EwoutH merged commit 778f6a6 into projectmesa:main Dec 1, 2024
12 checks passed
@EwoutH
Copy link
Member

EwoutH commented Dec 1, 2024

Having your first PR go through without any review comments is something that doesn't happen that often, great work.

@Sahil-Chhoker
Copy link
Contributor Author

Thanks @EwoutH, I just happened to be familiar with code.

AdamZh0u pushed a commit to AdamZh0u/mesa that referenced this pull request Dec 2, 2024
## Summary
Fixed the handling of empty iterables in _make_model_kwargs. Now raises a ValueError when an empty list is passed.

## Bug / Issue
Empty iterables weren't properly handled.

## Implementation
Updated `_make_model_kwargs` to raise a ValueError for empty iterables.
added a new function `test_batch_run_with_params_with_empty_content()` for testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch_Run begins, performs no iterations, no errors, writes empty .csv file. Help?
3 participants