-
Notifications
You must be signed in to change notification settings - Fork 894
Add typing and type annotations for icbc #1453
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: master
Are you sure you want to change the base?
Add typing and type annotations for icbc #1453
Conversation
Saransh-cpp
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.
Some suggestions 🙂
Given that DeepXDE supports only Python 3.8+, we can use -
from __future__ import annotationsto stay up to date!
I have made a few suggestions regarding the comment above - the rest of the types can be revamped accordingly too!
Also, mypy should be added in the CI to make sure that the types are correct and are being respected throughout the codebase. You can add mypy's configuration in pyproject.toml. This can maybe go in another PR.
(PS: these are just suggestions, the final call would be of DeepXDE's maintainers.)
deepxde/icbc/boundary_conditions.py
Outdated
| import numbers | ||
| from abc import ABC, abstractmethod | ||
| from functools import wraps | ||
| from typing import Any, Callable, List, Optional, overload, Tuple, Union |
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.
Given that DeepXDE supports Python 3.8+, you should use -
from __future__ import annotations
from typing import Any, Callable, Optional, overload
deepxde/icbc/boundary_conditions.py
Outdated
| outputs: Tensor, | ||
| beg: int, | ||
| end: int, | ||
| aux_var: Union[NDArray[np.float_], None] = None, |
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 should work once the suggestion above is applied.
| aux_var: Union[NDArray[np.float_], None] = None, | |
| aux_var: NDArray[np.float_] | None = None, |
deepxde/icbc/boundary_conditions.py
Outdated
| geom: Geometry, | ||
| func: Callable[[NDArray[np.float_]], NDArray[np.float_]], | ||
| on_boundary: Callable[[NDArray[Any], NDArray[Any]], NDArray[np.bool_]], | ||
| component: Union[List[int], int] = 0, |
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.
| component: Union[List[int], int] = 0, | |
| component: list[int] | int = 0, |
deepxde/types.py
Outdated
|
|
||
| # Tensor from any backend | ||
| Tensor = TypeVar("Tensor") | ||
| TensorOrTensors = Union[Tensor, Sequence[Tensor]] No newline at end of file |
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.
| TensorOrTensors = Union[Tensor, Sequence[Tensor]] | |
| TensorOrTensors = Tensor | Sequence[Tensor] | |
deepxde/types.py
Outdated
| @@ -0,0 +1,7 @@ | |||
| import numpy as np | |||
| from typing import Callable, Dict, List, Optional, Sequence, Tuple, Union, TypeVar | |||
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.
| from typing import Callable, Dict, List, Optional, Sequence, Tuple, Union, TypeVar | |
| from __future__ import annotations | |
| from typing import Sequence, TypeVar |
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.
Thank you for your suggestions, I will change them later 👍
|
Good suggestions @Saransh-cpp |
|
@supercgor @Saransh-cpp Any idea about this PR? |
Saransh-cpp
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.
@lululxvi this looks almost ready, but needs to be polished for merging. It would be great if @supercgor can pick this up again :)
| from ..types import Tensor, dtype, SparseTensor, TensorOrTensors | ||
| from numpy.typing import NDArray, ArrayLike |
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.
These imports (the heavy ones) should be under an if typing.TYPE_CHECKING.
|
|
||
|
|
||
| def is_tensor(obj): | ||
| def is_tensor(obj: object) -> bool: |
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.
Using object in type hints is not useful + not a very common practice. You would want to narrow this down ideally (because it is as helpful as not having a type hint at the moment).
| from numbers import Number | ||
|
|
||
| import numpy as np | ||
| from numpy.typing import NDArray |
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.
Should be moved in an if condition.
| from typing import Any, Literal | ||
|
|
||
| import numpy as np | ||
| from numpy.typing import ArrayLike, NDArray |
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.
Ditto
| from .sampler import sample | ||
| from .. import backend as bkd | ||
| from .. import config | ||
| from ..types import Tensor |
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.
Ditto
| self.x3_tensor = bkd.as_tensor(self.x3) | ||
|
|
||
| diff_x1_x2 = diff_x1_x3 = diff_x2_x3 = None | ||
| # diff_x1_x2 = diff_x1_x3 = diff_x2_x3 = 0.0 |
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.
There are a couple of code (logic) changes in this PR, which should not be the case.
|
|
||
| class GeometryXTime: | ||
| def __init__(self, geometry, timedomain): | ||
| class GeometryXTime(): |
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.
| class GeometryXTime(): | |
| class GeometryXTime: |
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 should not call this file as types as types is a builtin Python module.
| [mypy] | ||
| python_version = 3.8 | ||
| strict = false | ||
| ignore_missing_imports = true |
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.
| ignore_missing_imports = true | |
| ignore_missing_imports = true | |
creating an extra file called
types.py, referred from torch.