Skip to content

Conversation

@hheei
Copy link
Contributor

@hheei hheei commented Aug 21, 2023

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

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]] No newline at end of file
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

@lululxvi
Copy link
Owner

lululxvi commented Apr 7, 2025

@supercgor @Saransh-cpp Any idea about this PR?

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.

@lululxvi this looks almost ready, but needs to be polished for merging. It would be great if @supercgor can pick this up again :)

Comment on lines +5 to +6
from ..types import Tensor, dtype, SparseTensor, TensorOrTensors
from numpy.typing import NDArray, ArrayLike
Copy link
Contributor

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

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

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

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

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

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():
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
class GeometryXTime():
class GeometryXTime:

Copy link
Contributor

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
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
ignore_missing_imports = true
ignore_missing_imports = true

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