-
Notifications
You must be signed in to change notification settings - Fork 944
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
new feature: AgentSet.get can retrieve one or more then one attribute #2044
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
a510b34
new feature: AgentSet.get can retrieve one or more then one attribute
quaquel cb78523
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 22cd104
minor docstring update
quaquel 5f96227
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 76bb633
add AttributeError to docstring and fixes capital L in List
quaquel c0660a4
Update agent.py
quaquel c441910
Update agent.py
quaquel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
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.
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.
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 returnundefined
and not raise an error. However, Python doesn't distinguish betweennull
andundefined
, and so there is an information loss when returningNone
. It is ambiguous and could mean the modeler intentionally chooses to returnNone
instead of an absence of attribute.I prefer no fallback to
None
.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.
Maybe we could add a boolean switch (something like
allow_undefined
) that defaults toFalse
. WhenFalse
it isn't allowed and an error is returned, when True it is allowed andNone
values are returned.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.
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.
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.
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.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.
implementation wise this is a bit tricky.
default
ingetattr
is not a keyword argument but an argument. So you cannot useNone
to signal you don't specify default.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.
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
.