mesa icon indicating copy to clipboard operation
mesa copied to clipboard

Fixed batch_run accepting empty iterables

Open its-nmt05 opened this issue 1 year ago • 4 comments

Description

This PR addresses an issue in the batch_run method where parameters must be iterable and non-empty.

Changes

Added iterable and non-empty checks in _make_model_kwargs. Updated batch_run to handle only valid parameters.

Issue Reference

Closes #2108

its-nmt05 avatar Jun 22 '24 15:06 its-nmt05

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 -0.9% [-1.2%, -0.6%] 🔵 -0.9% [-1.1%, -0.7%]
Schelling large 🔵 -0.3% [-0.8%, +0.3%] 🔵 +1.0% [-1.5%, +3.7%]
WolfSheep small 🔵 -0.1% [-0.2%, +0.1%] 🔵 +0.1% [-0.1%, +0.2%]
WolfSheep large 🔵 +0.3% [-0.2%, +0.8%] 🔵 +0.6% [+0.0%, +1.2%]
BoidFlockers small 🔵 -1.1% [-1.7%, -0.5%] 🔵 -1.0% [-1.8%, -0.2%]
BoidFlockers large 🔵 -1.4% [-1.9%, -0.9%] 🔵 -1.0% [-1.9%, -0.1%]

github-actions[bot] avatar Jun 22 '24 15:06 github-actions[bot]

@its-nmt05 thank you for your contribution. Upon testing via the bank reserves example, I got this error

Traceback (most recent call last):
  File "/projectmesa/mesa-examples/examples/bank_reserves/batch_run.py", line 190, in <module>
    data = mesa.batch_run(
           ^^^^^^^^^^^^^^^
  File "/projectmesa/mesa/mesa/batchrunner.py", line 67, in batch_run
    data = process_func(run)
           ^^^^^^^^^^^^^^^^^
  File "/projectmesa/mesa/mesa/batchrunner.py", line 138, in _model_run_func
    model = model_cls(**kwargs)
            ^^^^^^^^^^^^^^^^^^^
  File "/projectmesa/mesa-examples/examples/bank_reserves/batch_run.py", line 157, in __init__
    for i in range(self.init_people):
             ^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'list' object cannot be interpreted as an integer

rht avatar Jun 24 '24 01:06 rht

It's because I intentionally set init_people to []. I think the correct handling should be to raise error instead of silently passing along the values. In the error message, you should say that if the param values is empty, it should be dropped altogether by the user by hand.

rht avatar Jun 24 '24 01:06 rht

Yeah I agree, explicit is better than implicit.

We should also document which values we allow and which we don't (like an empty list or not).

EwoutH avatar Jul 03 '24 11:07 EwoutH

superseded by #2523

quaquel avatar Dec 05 '24 06:12 quaquel