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

Add typing and type annotations for icbc #1453

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

supercgor
Copy link
Contributor

creating an extra file called types.py, referred from torch.

Copy link
Contributor

@Saransh-cpp Saransh-cpp left a 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 annotations

to 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.)

@@ -14,27 +14,37 @@
import numbers
from abc import ABC, abstractmethod
from functools import wraps
from typing import Any, Callable, List, Optional, overload, Tuple, Union
Copy link
Contributor

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

outputs: Tensor,
beg: int,
end: int,
aux_var: Union[NDArray[np.float_], None] = None,
Copy link
Contributor

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.

Suggested change
aux_var: Union[NDArray[np.float_], None] = None,
aux_var: NDArray[np.float_] | None = None,

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from typing import Callable, Dict, List, Optional, Sequence, Tuple, Union, TypeVar
from __future__ import annotations
from typing import Sequence, TypeVar

Copy link
Contributor Author

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 👍

@lululxvi
Copy link
Owner

Good suggestions @Saransh-cpp

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.

3 participants