-
Notifications
You must be signed in to change notification settings - Fork 927
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
DataCollector: Allow agent reporters to take class methods and functions with parameter lists #1838
DataCollector: Allow agent reporters to take class methods and functions with parameter lists #1838
Conversation
…ons with parameter lists Modify the `DataCollector` class to allow agent reporters to take methods of a class/instance and functions with parameters placed in a list (like model reporters), by extending the `_new_agent_reporter` method. This implementation starts by checking if the reporter is an attribute string. If so, it creates a function to retrieve the attribute from an agent. Next, it checks if the reporter is a list. If it is, this indicates that we have a function with parameters, so it wraps that function to pass those parameters when called. For any other type (like lambdas or methods), we assume they're directly suitable to be used as reporters. Now, with this modification, agent reporters in the `DataCollector` class can take: 1. Attribute strings 2. Function objects (like lambdas) 3. Methods of a class/instance 4. Functions with parameters placed in a list This approach ensures backward compatibility because the existing checks for attribute strings and function objects remain unchanged. The added functionality only extends the capabilities of the class without altering the existing behavior.
Is this a syntax sugar for an agent reporter of previously specified as |
_new_agent_reporter wasn't intended into the DataCollector class, so thus not seen as a method.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1838 +/- ##
==========================================
+ Coverage 79.01% 79.12% +0.11%
==========================================
Files 15 15
Lines 910 915 +5
Branches 193 194 +1
==========================================
+ Hits 719 724 +5
Misses 168 168
Partials 23 23
☔ View full report in Codecov by Sentry. |
70edb80
to
2a266b2
Compare
- Move agent_reporters specification into initialize_data_collector() - Add back the tables argument (accidentally deleted in previous commit) - Use parentheses to parse step and agent_id from agent records dataframe, since those are the multi-index key - Update expected values for new agent reporter types - Update length values of new agent table and vars (both increase by 2 due to 2 new agent reporter columns)
2a266b2
to
05d1767
Compare
Code is ready to review. Will still work a bit on documentation.
Yes, that's completely right, the modification is essentially syntactic sugar, offering an alternative way to specify agent reporters that involve functions with additional parameters. The big advantage of this however, is that it's now completely consistent with specifying model reporters. This way, if you can use the syntax for a model reporter, you can use it for an agent reporter, and visa versa. A disadvantage could of course be the increased complexity of the code base. But, then I would argue, we should also report one of those syntax types from the model reporter. Because than that would be consistent, but I also don't think that's a desired option due to breaking backwards compatibility. |
Updated the docs, this PR is ready for review. I might update the tutorial and some examples in separate PRs. |
I personally think that passing I think it has nothing to do with the pickling issue in @jackiekazil @tpike3 should we merge this PR? |
return getattr(agent, attribute_name, None) | ||
|
||
reporter.attribute_name = attribute_name |
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.
This seems to be a bug introduced in bc5a5cd, and you fixed in this PR.
Content: LGTM. Concept: I prefer passing |
I am good with this. @rht thank you for doing the review. |
Thanks for reviewing and merging! |
Thinking about it a bit longer I think I just found quite a serious limitation to this approach. Take the class method syntax: agent_reporters={
"double_value": MockAgent.double_val,
}, This works perfectly fine for models with one Agent class, but what about models with multiple agent types? We can't use We should consider this syntax and how to handle it in #348 #1142 #1419 #1701 #1702 or wherever our current multiple agent-type datacollection discussion is taking place. |
Modify the
DataCollector
class to allow agent reporters to take methods of a class/instance and functions with parameters placed in a list (like model reporters), by extending the_new_agent_reporter
method.This implementation starts by checking if the reporter is an attribute string. If so, it creates a function to retrieve the attribute from an agent. Next, it checks if the reporter is a list. If it is, this indicates that we have a function with parameters, so it wraps that function to pass those parameters when called.
For any other type (like lambdas or methods), we assume they're directly suitable to be used as reporters.
Now, with this modification, agent reporters in the
DataCollector
class can take:This approach ensures backward compatibility because the existing checks for attribute strings and function objects remain unchanged. The added functionality only extends the capabilities of the class without altering the existing behavior.
Closes: #1837
Todo
Can be done in separate PRs: