new feature: AgentSet.get can retrieve one or more then one attribute

[quaquel]

This adds a small additional feature to AgentSet. The get method now takes a string or list of strings. This allows you to retrieve multiple attributes in one go. I also added a test to cover this new feature.

[github-actions]

Performance benchmarks:

Model	Size	Init time [95% CI]	Run time [95% CI]
Schelling	small	🔵 -0.4% [-0.7%, -0.1%]	🔵 +0.5% [+0.3%, +0.6%]
Schelling	large	🔵 +11.0% [-18.0%, +50.3%]	🔵 -0.9% [-1.9%, -0.3%]
WolfSheep	small	🔵 -1.4% [-1.7%, -0.9%]	🔵 -0.2% [-0.3%, -0.0%]
WolfSheep	large	🔵 -1.4% [-2.4%, -0.6%]	🔵 -0.9% [-1.4%, -0.6%]
BoidFlockers	small	🔵 -2.1% [-2.6%, -1.5%]	🔵 +0.8% [+0.1%, +1.6%]
BoidFlockers	large	🔵 -0.8% [-1.4%, -0.2%]	🔵 +2.9% [+2.4%, +3.4%]

[EwoutH]

Thanks, looks useful! One minor questions on the output.

[EwoutH]

Is this still correct? Would it be nested in some sort when a list of attrs is inputted?

Maybe make a little bit more explicit what is returned when a str is inputted for attr_names, and what when a list of strings is inputted.

[quaquel]

see the updated text

[EwoutH]

Would returning a dict with the attr_names as keys and a list of agent values as value be more elegant?

return_dict = {
    "attr_name1": [value1, value2],
    "attr_name2": [value1, value2]
}

[quaquel]

I went back and forth on that. In the end, I stuck with the current design which also does not return a dict in case of just one attribute. Moreover, the caller knows the attributes and their order so it has all the information it needs and can easily create a list of dicts if desired.

Also, I prefer to keep the agent order in the return first. Long story short: this is in line with e.g. numpy which is row major in how the data is structured.

[Corvince]

Maybe we should default to None (getattr(agent, attr_names, None)). This way it would be easier to collect across different agent types, which might or might not have the specified attributes.

[quaquel]

I see your point but I am torn. I personally probably would prefer getting the error explicitly rather than being confronted with None values downstream.

[rht]

Past discussion: projectmesa/mesa-examples#90. The summary is that it depends on the coder's background: in JavaScript, it is not surprising for obj.attr to return undefined and not raise an error. However, Python doesn't distinguish between null and undefined, and so there is an information loss when returning None. It is ambiguous and could mean the modeler intentionally chooses to return None instead of an absence of attribute.
I prefer no fallback to None.

[EwoutH]

Maybe we could add a boolean switch (something like allow_undefined) that defaults to False. When False it isn't allowed and an error is returned, when True it is allowed and None values are returned.

[quaquel]

that would make the code internally more complicated. You would get two additional nested if/else.

The easier solution is for the user to do agentset.select("condition to ensure attribute exists").get().

Also, why allow None in the first place? It only gives more downstream problems.

[Corvince]

I think its not necesarily an "error" if we collect None instead of triggering an AttributeError. It depends on the use case. If you quickly want to get some attributes on all of your agents, but some agents don't have that attribute you still get some result. Otherwise you first have to pre-select the right AgentSet and then collect the attributes.

I think the cleanest solution would probably be to mimic getattr in that users can provide a default value for agentset.get() and pass that to getattr, if set.

[quaquel]

implementation wise this is a bit tricky. default in getattr is not a keyword argument but an argument. So you cannot use None to signal you don't specify default.

[rht]

We can defer this discussion and implementation in a separate PR. The current behavior in this PR is consistent with the existing API, which does not fall back to undefined/None.

[EwoutH]

Since the output format is different, and there are two separate code paths anyway, I was also thinking if it shouldn't be a separate method. Something like get_multiple or so.

[quaquel]

I came to this code while playing with the data collection. Somewhere you have to go down either path (str, list of str). In the end, I saw pushing it into the agentset was the most convenient, but I am fine by separating it here into 2 methods and moving the if upwards

[rht]

The List here shouldn't be capitalized.

[rht]

The return value also has a capitalized List. This may be a small typo, but it can't be automatically fixed by pyupgrade.

[rht]

Other than the List typo, LGTM.

[quaquel]

Other than the List typo, LGTM.

The List typo is fixed and I made the attribute error behavior explicit in the docstring

[rht]

The List typo is fixed and I made the attribute error behavior explicit in the docstring

I still see capitalized List, and the last commit 76bb633 only added the AttributeError documentation.

[quaquel]

Oh, you meant in the docstring?

[rht]

Yes, that's why pyupgrade can't parse it, because it's a string. There is 1 more: https://github.com/projectmesa/mesa/pull/2044/files#diff-1dffe661c1ef6a8c750afb07bd029251b7c35f7793b044c5dde893c59236f0c3R272.

[quaquel]

Yes, that's why pyupgrade can't parse it, because it's a string. There is 1 more: https://github.com/projectmesa/mesa/pull/2044/files#diff-1dffe661c1ef6a8c750afb07bd029251b7c35f7793b044c5dde893c59236f0c3R272.

And they are a pain to find in the IDE. But its fixed now.