Skip to content
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

type annotations: Replace Union[a, b] with a | b, Optional[a] with a | None #1341 #2003

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Aryan1Mahahjan
Copy link

Solving the issue of replacing Union[a, b] with a | b and Optional[a] with a | None in type annotations:

Replacing Union[a, b] with a | b:

Improves readability and clarity of type hints.
Makes it easier to understand what types are allowed for a variable or function parameter.
Provides better type checking by the static type checker.

It may CI issuse in build / build (ubuntu, 3.9) (pull_request)

Aryan1Mahajahn

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 -1.3% [-1.6%, -1.1%] 🔵 -0.7% [-0.9%, -0.6%]
Schelling large 🔵 -0.2% [-0.7%, +0.2%] 🔵 -0.6% [-2.6%, +1.4%]
WolfSheep small 🔵 +1.7% [+1.4%, +2.1%] 🔵 -1.5% [-1.6%, -1.4%]
WolfSheep large 🔵 +3.4% [+0.8%, +5.8%] 🔵 +1.7% [+1.0%, +2.4%]
BoidFlockers small 🔵 +3.5% [+2.5%, +4.4%] 🔵 -0.8% [-1.6%, +0.0%]
BoidFlockers large 🔴 +4.4% [+3.9%, +4.8%] 🔵 -1.6% [-2.1%, -1.2%]

Copy link
Contributor

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from two minor things, looks good!

Now we just have to wait until we drop Python 3.9 support. I think that will happen after the Mesa 2.3 release, which will probably release in February or March. If you want, you can help with that when the time comes.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
mesa/time.py Outdated
Comment on lines 24 to 25
# Mypy; for the `|` operator purpose
# Remove this __future__ import once the oldest supported Python is 3.10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines can also be removed

@rht
Copy link
Contributor

rht commented Jan 26, 2024

Now we just have to wait until we drop Python 3.9 support. I think that will happen after the Mesa 2.3 release, which will probably release in February or March. If you want, you can help with that when the time comes.

What's blocking us from doing it for Mesa 2.3? See also #1756 (comment).

@rht rht mentioned this pull request Jan 26, 2024
@Aryan1Mahahjan
Copy link
Author

Hi, is there any way I can contribute in any similar issue.

@EwoutH
Copy link
Contributor

EwoutH commented Jan 30, 2024

I think improving the example models in Mesa-examples and adding new ones is now the most useful. Especially if you include new features like the AgentSet or PropertyLayer.

@rht
Copy link
Contributor

rht commented Jan 30, 2024

Adding more Mypy annotations would also be useful.

@rht
Copy link
Contributor

rht commented Feb 1, 2024

@Aryan1Mahahjan here are several type checking errors for mesa/agent.py

mesa/agent.py:45: error: Type argument "float" of "NDArray" must be a subtype of "generic"  [type-var]
mesa/agent.py:79: error: "Model" has no attribute "random"  [attr-defined]
mesa/agent.py:139: error: Argument 1 of "__contains__" is incompatible with supertype "AbstractSet"; supertype defines the argument type as "object"  [override]
mesa/agent.py:139: note: This violates the Liskov substitution principle
mesa/agent.py:139: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
mesa/agent.py:139: error: Argument 1 of "__contains__" is incompatible with supertype "Sequence"; supertype defines the argument type as "object"  [override]
mesa/agent.py:139: error: Argument 1 of "__contains__" is incompatible with supertype "Container"; supertype defines the argument type as "object"  [override]
mesa/agent.py:200: error: "WeakKeyDictionary[Agent, None]" has no attribute "data"  [attr-defined]
mesa/agent.py:279: error: Signature of "__getitem__" incompatible with supertype "Sequence"  [override]
mesa/agent.py:279: note:      Superclass:
mesa/agent.py:279: note:          @overload
mesa/agent.py:279: note:          def __getitem__(self, int, /) -> Any
mesa/agent.py:279: note:          @overload
mesa/agent.py:279: note:          def __getitem__(self, slice, /) -> Sequence[Any]
mesa/agent.py:279: note:      Subclass:
mesa/agent.py:279: note:          def __getitem__(self, int | slice, /) -> Agent
mesa/agent.py:289: error: Incompatible return value type (got "Agent | list[Agent]", expected "Agent")  [return-value]
mesa/agent.py:359: error: "Model" has no attribute "random"  [attr-defined]

are you interested in looking to fixing it? You should start with opening a PR with small changes first, so that it can get merged earlier.

@Aryan1Mahahjan
Copy link
Author

Yes I am interested I will look into it.

@Aryan1Mahahjan
Copy link
Author

Aryan1Mahahjan commented Feb 8, 2024

image

It works on my machine no errors found

@rht
Copy link
Contributor

rht commented Feb 8, 2024

You do type checking by installing Mypy and running mypy <the file.py> or mypy thedirectory/.

@quaquel quaquel mentioned this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants