-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix #2716: missing agent type hints #2885
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
base: main
Are you sure you want to change the base?
fix #2716: missing agent type hints #2885
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Thanks for tackling this issue, improved type hints are definitely useful. The implementation looks solid, but I wanted to mention: since we recently dropped Python 3.11 support and now require 3.12+, we can actually use the newer PEP 695 syntax instead of the Python 3.12 introduced cleaner syntax for generics that eliminates the need for the conditional Instead of: if TYPE_CHECKING:
M = TypeVar("M", bound=Model)
A = TypeVar("A", bound="Agent")
else:
M = TypeVar("M")
A = TypeVar("A")
class Agent(Generic[M]): # noqa: UP046
def __init__(self, model: M, *args, **kwargs) -> None:
self.model: M = modelWe can now write: class Agent[M: Model]:
def __init__(self, model: M, *args, **kwargs) -> None:
self.model: M = modelSame goes for class AgentSet[A: Agent](MutableSet[A], Sequence[A]):
# ...
@classmethod
def create_agents[T: Agent](cls: type[T], model: M, n: int, *args, **kwargs) -> AgentSet[T]:
# ...The PEP 695 syntax is more readable, eliminates the runtime/type-checking split, and is the idiomatic way to do this in Python 3.12+. The Would you be up for refactoring this to use the new syntax? |
|
@EwoutH Thanks, i have made the following changes |
EwoutH
left a comment
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.
Thanks! Few minor points
|
|
||
| import numpy as np | ||
|
|
||
| if TYPE_CHECKING: |
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.
Could you check if we still need this? (honestly I don't know)
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.
As far as I went through the codebase, I think numpy is needed
Example usecase (Line 146)
def rng(self) -> np.random.Generator:
"""Return a seeded np.random rng."""
return self.model.rng
- for the TYPE_CHECKING, i have used it to prevent circular imports, do let me know if i am wrong in any of the both cases
mesa/agent.py
Outdated
| # Preserve the more specific model type for static type checkers. | ||
| # At runtime this remains the Model instance passed in. |
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.
Comments are not needed
| # Preserve the more specific model type for static type checkers. | |
| # At runtime this remains the Model instance passed in. |
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.
ok, I will make the neccessary changes
mesa/model.py
Outdated
| type[Agent], AgentSet | ||
| ] = {} # a dict with an agentset for each class of agents | ||
| self._all_agents = AgentSet( | ||
| [], random=self.random | ||
| ) # an agenset with all agents |
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.
please restore the original comments
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.
Made the changes, please review the new push
|
@EwoutH incase you missed the changes, could you look upon the changes |
| def create_agents[T: Agent]( | ||
| cls: type[T], model: Model, n: int, *args, **kwargs | ||
| ) -> AgentSet[T]: |
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.
Why is T used here for Agent? Wouldn't A be more logical?
Please motivate first. But if you agree:
| def create_agents[T: Agent]( | |
| cls: type[T], model: Model, n: int, *args, **kwargs | |
| ) -> AgentSet[T]: | |
| def create_agents[A: Agent]( | |
| cls: type[A], model: Model, n: int, *args, **kwargs | |
| ) -> AgentSet[A]: |
| inplace: bool = False, | ||
| agent_type: type[Agent] | None = None, | ||
| agent_type: type[A] | None = None, | ||
| ) -> AgentSet: |
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 also add a type on the return, right?
| ) -> AgentSet: | |
| ) -> AgentSet[A]: |
| if TYPE_CHECKING: | ||
| pass | ||
|
|
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 empty block can be removed, right?
| if TYPE_CHECKING: | |
| pass |
|
|
||
| # mypy | ||
| from typing import Any | ||
| from typing import TYPE_CHECKING, Any |
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.
Can be removed I think.
| from typing import TYPE_CHECKING, Any | |
| from typing import Any |
| return ( | ||
| AgentSet(sorted_agents, self.random) | ||
| if not inplace | ||
| else self._update(sorted_agents) |
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.
In the sort function, [A] can also be added.
def sort(
self,
key: Callable[[Agent], Any] | str,
ascending: bool = False,
inplace: bool = False,
) -> AgentSet[A]:| self.step = self._wrapped_step | ||
|
|
||
| # setup agent registration data structures | ||
| self._agents = {} # the hard references to all agents in the model |
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 this is possible:
| self._agents = {} # the hard references to all agents in the model | |
| self._agents = dict[A, None] # the hard references to all agents in the model |
129f3c2 to
638dc87
Compare
|
@SiddharthBansal007 sorry for the late review. I've a few more (minor) comments, can you address them? Please don't adopt them without thinking, but critically check. |
Add generic typing to Agent, AgentSet, and Model classes
Implement TypeVar-based generics to enable static type checkers to reason about
concrete agent and model types throughout the codebase. This resolves typing gaps
where model.agents returned untyped AgentSet[Agent] and self.model did not
preserve model-specific type information.
Key changes:
Type checking benefits:
Implementation details:
Testing:
Addresses: Typing improvements for better IDE support and static analysis