Enhance AgentSet.get to handle undefined attributes with fallback opt… #2067
Conversation
…ions These chanegs provides a more user friendly approach to attribute retrieval, accommodating scenarios where some agents might not have all the specified attributes.
for more information, see https://pre-commit.ci
|
Performance benchmarks:
|
|
I think the pr generally looks good, thanks for the contribution! But I still think we can get away with just having a fallback value. The way this works is we have to set it's default value to a sentinel value and then check if the value is still the sentinel value and if not, return the fallback value. This way the fallback value can also be None. The sentinel value could look just like sentinel = object() |
|
I like the basic proposed solution with the inner function. This is quite elegant. A small addition from my side: could you add some unit tests as well? The test code is here. |
mesa/agent.py
Outdated
| """ | ||
| Retrieve the specified attribute(s) from each agent in the AgentSet. | ||
|
|
||
| Args: | ||
| attr_names (str | list[str]): The name(s) of the attribute(s) to retrieve from each agent. | ||
| handle_undefined (bool, optional): If False, use fallback_value for undefined attributes. Defaults to True. |
There was a problem hiding this comment.
I think it would be clearer is it is called error_when_undefined, or if there is a more concise naming for it.
There was a problem hiding this comment.
Hi @rht ,
Made some changes into the latest commit could you please have a look and let me know if is there anything to change :)
Hi there, |
Hi @Corvince , Could you please have a look at the latest commit and let me know if is that fine, let me know... |
|
Hi, yes! Thats exactly how I meant it. Now you could remove the "error_when_undefined" boolean, because you can either raise an Error if the fallback_value is undefined or use the fallback value. I also think its not necessary to include the sentinel value in the docstrings and documentation. Its purely an implementation detail and not something the user needs to be aware of. So its enough to state that, if provided, the fallback value will be used if an attribute error is encountered and otherwise an error is raised. |
|
You might be wondering: If you leave fallback_value undefined and turn off error when undefined, it might appear the code is just returning nothing. But it will actually return None, so its the same as setting fallback_value to None, so error_when_undefined doesn't provide any additional functionality and thats why I think it can be removed. |
Yeah, you are right! |
for more information, see https://pre-commit.ci
Hey I have done some changes according to your preference.Let me know if its perfect or not... Thank you 👍 |
mesa/agent.py
Outdated
| @@ -90,7 +90,7 @@ class AgentSet(MutableSet, Sequence): | |||
|
|
|||
| Methods: | |||
| __len__, __iter__, __contains__, select, shuffle, sort, _update, do, get, __getitem__, | |||
| add, discard, remove, __getstate__, __setstate__, random | |||
| add, discard, remove, __getstate__, __setstate__, random, group_by | |||
There was a problem hiding this comment.
I guess this is from another branch?
There was a problem hiding this comment.
I think thats was a mistake by me. Let me fix it :)
mesa/agent.py
Outdated
| def get_attr(agent, attr_name): | ||
| try: | ||
| return getattr(agent, attr_name) | ||
| except AttributeError: |
There was a problem hiding this comment.
Catching an exception is comparatively costly in terms of performance. Normally this isn't something to worry about, but if we run into this frequently (which users might do) it could become a problem. I think it would be better to first check for the sentinel value and then either return getattr(agent, attr_name) or getattr(agent, attr_name, fallback_value).
There was a problem hiding this comment.
Hey facing some attribute error in the build after the changes, any ideas.....
Almost 🙈 One more comment/consideration |
fix the typo and remove exception and handle the exception using if else
for more information, see https://pre-commit.ci
Hi could you please have a look at the recent commit and let me know if is there any issue... |
All existing tests are failing, and no tests have been added for the new functionality. Both issues need addressing. If you need help or tips, please let us know. |
Actually after this commit(fe6fea3) all test are failing. And regardin the test need to add for the changes I will add that shortly after the build is succeed. Sorry for the inconvenience :( |
|
@EwoutH, |
|
Yes, it seems that currently one of the tests fail. It’s Line 213 in be5fc01 Most likely a failing test points to a break in current functionality. But it could also be that the test is unnecessary (strict). so either the code needs to be fixed to pass the test, or the test modified. I hope this helps. If you have further questions (or want me to just fix the test). You will learn a lot working through these issues though! |
Yeah, Let me try to fix that issue ... |
Issue no : #2045
These chanegs provides a more user friendly approach to attribute retrieval, accommodating scenarios where some agents might not have all the specified attributes.