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

Enable type checking #340

Merged
merged 3 commits into from
Jun 28, 2022
Merged

Enable type checking #340

merged 3 commits into from
Jun 28, 2022

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented Jun 28, 2022

Splitting this out from #308 to keep the discussion separate.

Comment on lines -1290 to +1298
A: Optional[Union[Real, np.ndarray]] = None, # noqa: N803
b: Optional[Union[Real, np.ndarray]] = None):
A: Optional[Union[np.generic, np.ndarray]] = None, # noqa: N803
b: Optional[Union[np.generic, np.ndarray]] = None):
"""Apply the affine map :math:`f(x) = A x + b` to the geometry of *mesh*."""

if isinstance(A, Real):
if A is not None and not isinstance(A, np.ndarray):
A = np.diag([A] * mesh.ambient_dim) # noqa: N806

if isinstance(b, Real):
if b is not None and not isinstance(b, np.ndarray):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@inducer I had a little trouble here: the type checking for the numpy functions doesn't like Real. I get this error with the original code:

meshmode/mesh/processing.py:1295: error: Argument 1 to "diag" has incompatible type "List[Real]"; expected "Union[_SupportsArray[dtype[Any]], Sequence[_SupportsArray[dtype[Any]]], Sequence[Sequence[_SupportsArray[dtype[Any]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]]]]"  [arg-type]

The change fixes it, but I'm not sure if there's something more specific I can use instead of np.generic (or if this can be written in a different way to avoid the issue).

Copy link
Owner

Choose a reason for hiding this comment

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

I think this is OK for now. Mypy is still at war with Python's numerical tower, cf. python/mypy#3186 for example. I expect that this will get better with time.

@majosm majosm marked this pull request as ready for review June 28, 2022 18:07
@majosm majosm requested a review from inducer June 28, 2022 18:07
@inducer
Copy link
Owner

inducer commented Jun 28, 2022

LGTM, thanks! (And I've marked mypy required.)

@inducer inducer merged commit 0d22588 into inducer:main Jun 28, 2022
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.

2 participants