Skip to content

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