-
Notifications
You must be signed in to change notification settings - Fork 929
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
Enhance DataCollector to validate model_reporters functions #2605
base: main
Are you sure you want to change the base?
Enhance DataCollector to validate model_reporters functions #2605
Conversation
Thanks for the PR, sounds interesting. What's the performance overhead? |
f"Warning: Lambda reporter '{name}' failed: {e!s}\n" | ||
f"Example of valid lambda: lambda m: len(m.agents)", | ||
UserWarning, | ||
stacklevel=2, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why issue a warning instead of an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why issue a warning instead of an exception?
Thanks for the detailed suggestions for my revised codes! During this time, I try to understand them.
About the two questions:
- Yes, the mechanism of the expectation can avoid serious problems when using the function of the datacollector. As it's my first time adding the validate function, I chose the soft way to validate: just a warning not to influence the mesa code of the original version.
I had tried to write a new version of codes, which are all replaced with the expectation instead of the warning.
# Type 1: Lambda function
if isinstance(reporter, types.LambdaType):
try:
reporter(model)
except Exception as e:
raise RuntimeError(
f"Lambda reporter '{name}' failed validation: {str(e)}\n"
f"Example: lambda m: len(m.agents)"
) from e
- You're right that validating every data collection could impact performance. I propose two potential solutions:
- (a) make the "
_validate_model_reporter
" function into the optional function, like:
def __init__(
self,
model_reporters=None,
agent_reporters=None,
agenttype_reporters=None,
tables=None,
validate_reporters=False, # Add new parameter
):
- (b) Move validation to initialization time:
Put the "_validate_model_reporter
" function in theClass DataCollector __init__()
and the "_validate_model_reporter
" function can be only run once, boosting the running time and lowing the need of performance.
Class DataCollector:
def __init__():
xxxxxxx...
if model_reporters is not None:
for name, reporter in model_reporters.items():
self._validate_reporter_format(name, reporter) # my new way
self._new_model_reporter(name, reporter)
I wonder what your opinions are. Thanks for your attention to this PR.
# Add validation | ||
self._validate_model_reporter(var, reporter, model) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that the validation is done every single time you try to collect the data? That seems inefficient and overkill.
Thanks for your attention to the PR. I apologize for the delayed response as I was unfamiliar with Mesa's testing logistics. Regarding the overhead:
Future considerations:
|
I suggest running the validation logic only on the first-ever call to |
Summary
This PR enhances the DataCollector class by adding a comprehensive validation system for model reporters. The main goal is to provide clearer feedback when reporters are misconfigured, while maintaining backward compatibility.
Motive
The original DataCollector implementation had several limitations:
These issues made debugging difficult and led to confusion when setting up model reporters.
Implementation
Usage Examples
Additional Notes
test_model_reporters.py
datacollection.py
: the only one py code changed in mesa offical repository.warnings
moduledatacollection_new.zip