Include python demos in mypy check#1003
Conversation
|
@schnellerhase Hi, I just want to make sure that the failing CI check isn't something I need to worry about. It looks like that same check has been failing on other pull requests recently and isn't related to these changes. Is there anything I need to modify? Thank you! |
| FloatArray = npt.NDArray[np.float64] | ||
| FloatingArray = npt.NDArray[np.floating] | ||
| QuadratureRule = tuple[FloatArray, FloatArray] | ||
|
|
There was a problem hiding this comment.
These are quite verbose changes to every demo for the sake of typing. What is the issue raised without them?
There was a problem hiding this comment.
Thanks for the feedback! The demos had 305 errors with the mypy check which I've attached in a text file: demo_mypy_errors.txt
The majority of errors across all of the demos come from Basix functions returning broad ArrayLike types (these functions include make_quadrature, create_lattice, tabulate, tabulate_polynomials). Mypy is treating these values as large unions (buffers, strings, nested sequences, etc.), which leads to errors when the demos use them as NumPy arrays, for example:
- slicing/indexing (
pts[:, 0],poly[i, :]) - arithmetic (
f * poly * wts) - passing them to functions expecting
NDArray
demo_custom_element.py had the most errors (199) which also included:
- Empty nested lists such as
x = [[], [], [], []]andM = [[], [], [], []]are inferred aslist[list[Never]], so mypy rejects later.append(np.array(...))calls and the latercreate_custom_element(..., x, M, ...)calls - There are also a few smaller API typing mismatches (using lists where tuples are expected).
The lines you commented on specifically are not necessary, I added them thinking it would keep things more readable but inline type casting would work just fine!
There was a problem hiding this comment.
Right thanks. These are however exactly the errors we would like to resolve with this extended check, so ignoring them by casting to the expected types is not the desired resolution strategy. They show that the current type hints are insufficient. Rather we want to ensure that the type hints provided align with the use cases in the demos. For an example to resolve the errors related to tabulate_polynomials and make_quadrature see https://github.com/FEniCS/basix/tree/schnellerhase/fix-some-types (feel free to incorporate).
No mypy checks are broken for unrelated reasons. |
|
@risa-hirsh, the |
|
Hi @risa-hirsh, thanks for opening your first pull request to Basix! I agree with @schnellerhase that it would be good to avoid adding too much typing to the demos. Do you want to have a go at updating the types of the functions as in main...schnellerhase/fix-some-types? |
| element = basix.create_custom_element( | ||
| CellType.quadrilateral, | ||
| [], | ||
| (), |
There was a problem hiding this comment.
This is a correction we should make.
Related: it would be good to update the basix functions to accept any Sequence type and convert to tuples inside the functions. I've opened #1010 for this and labelled it good first issue
4 participants
This pull request is for Issue #993 and extends mypy checks to cover the Python demo files and fixes type issues in those demos.
I made the following changes:
-Added demo/python to the mypy check
-Used typing.cast where necessary to handle ArrayLike return types
-Added minimal NumPy typing annotations to keep everything simple and readable
I didn’t make any changes to the logic of the demos. I compared the results from before and after making changes for every demo to ensure the results did not change.
I tested that mypy ran on demo/python successfully