-
Notifications
You must be signed in to change notification settings - Fork 162
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
refactor: Simplify Sugarscape resource agents #71
Conversation
I tested with the tests in #70, and confirmed that the resulting timeseries is unchanged. |
ee04ba1
to
d0351c0
Compare
this_cell = self.model.grid.get_cell_list_contents(pos) | ||
for agent in this_cell: | ||
if type(agent) is Sugar: | ||
if type(agent) is Resource: | ||
return agent | ||
return 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.
Sorry should be point to line 62/ 58 return None
--- I don't think this ever called. Under this set up there is a resource agent in every cell even if they have 0 sugar and 0 spice. If it was called line 260 and 261 would error out with None + int
Correct?
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 agree that the return None
not being called because every cells has a Resource
agent is rather an implicit consequence. A type checker would have said that self.get_resource(pos).sugar_amount
does not hold for all cases.
It should be replaced with a patch/cell object soon, so that the return value of get_resource
is Resource
, not Resource | 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.
I raised an Exception
in get_resource
when no Resource
agent is found. This should pass Mypy check.
This combines the Sugar and Spice resources into 1 resource class.
d0351c0
to
284ac47
Compare
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.
LGTM
This combines the Sugar and Spice resources into 1 resource class.
Rational: