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

type check in unit test #200

Merged
merged 2 commits into from
Sep 13, 2020
Merged

type check in unit test #200

merged 2 commits into from
Sep 13, 2020

Conversation

Trinkle23897
Copy link
Collaborator

@Trinkle23897 Trinkle23897 commented Sep 1, 2020

Fix #195: Add mypy test in .github/workflows/docs_and_lint.yml.

Also remove the out-of-the-date api

@Trinkle23897 Trinkle23897 marked this pull request as draft September 1, 2020 02:11
@Trinkle23897 Trinkle23897 linked an issue Sep 1, 2020 that may be closed by this pull request
tianshou/data/batch.py Show resolved Hide resolved
tianshou/policy/multiagent/mapolicy.py Outdated Show resolved Hide resolved
@Trinkle23897
Copy link
Collaborator Author

Trinkle23897 commented Sep 2, 2020

UPDATE: resolved

There are many errors like

tianshou/policy/imitation/base.py:45: error: Return type "Dict[str, float]" of "learn" incompatible with return type "Dict[str, Union[float, List[float]]]" in supertype "BasePolicy"  [override]

Possibly a bug in mypy:

from typing import Union

class A:
    def test(self) -> Union[float, int]:
        pass

class B(A):
    def test(self) -> int:
        pass

This works in mypy check. However,

from typing import Dict, Union

class A:
    def test(self) -> Dict[str, Union[float, int]]:
        pass

class B(A):
    def test(self) -> Dict[str, int]:
        pass

This cannot pass the check:

$ mypy try.py
try.py: note: In member "test" of class "B":
try.py:8: error: Return type "Dict[str, int]" of "test" incompatible with return type "Dict[str, Union[float, int]]" in supertype "A"  [override]
Found 1 error in 1 file (checked 1 source file)

However, here says narrow type in subclass is acceptable.

python/mypy#9418

tianshou/data/batch.py Outdated Show resolved Hide resolved
@Trinkle23897 Trinkle23897 marked this pull request as ready for review September 2, 2020 05:19
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2020

Codecov Report

Merging #200 into master will increase coverage by 0.32%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   94.09%   94.42%   +0.32%     
==========================================
  Files          40       39       -1     
  Lines        2457     2438      -19     
==========================================
- Hits         2312     2302      -10     
+ Misses        145      136       -9     
Flag Coverage Δ
#unittests 94.42% <95.45%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tianshou/exploration/__init__.py 100.00% <ø> (ø)
tianshou/env/worker/dummy.py 91.30% <83.33%> (ø)
tianshou/policy/modelfree/sac.py 87.50% <84.21%> (+1.04%) ⬆️
tianshou/env/worker/ray.py 85.29% <87.50%> (+2.43%) ⬆️
tianshou/env/worker/subproc.py 91.21% <88.46%> (+0.05%) ⬆️
tianshou/policy/modelfree/ppo.py 96.47% <92.85%> (-0.05%) ⬇️
tianshou/data/utils/segtree.py 70.27% <93.33%> (+4.64%) ⬆️
tianshou/exploration/random.py 97.29% <93.33%> (ø)
tianshou/policy/multiagent/mapolicy.py 93.75% <93.33%> (ø)
tianshou/data/collector.py 94.44% <95.45%> (+2.95%) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 380e9e9...62d875a. Read the comment docs.

duburcqa
duburcqa previously approved these changes Sep 6, 2020
docs/index.rst Outdated Show resolved Hide resolved
examples/box2d/README.md Outdated Show resolved Hide resolved
examples/box2d/README.md Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tianshou/data/batch.py Show resolved Hide resolved
tianshou/policy/modelfree/ddpg.py Outdated Show resolved Hide resolved
tianshou/policy/modelfree/sac.py Outdated Show resolved Hide resolved
tianshou/policy/multiagent/mapolicy.py Outdated Show resolved Hide resolved
tianshou/utils/moving_average.py Outdated Show resolved Hide resolved
tianshou/utils/moving_average.py Outdated Show resolved Hide resolved
@duburcqa
Copy link
Collaborator

duburcqa commented Sep 6, 2020

My overall comment is that typing checking should make things more robust and clearer. It is clearly not the case at the current point, so I do not recommand satisfying all its recommendations at this point. Maybe it will get better in the future but it does not look viable for now. Just my opinion though.

@Trinkle23897
Copy link
Collaborator Author

Trinkle23897 commented Sep 6, 2020

My overall comment is that typing checking should make things more robust and clearer. It is clearly not the case at the current point, so I do not recommand satisfying all its recommendations at this point. Maybe it will get better in the future but it does not look viable for now. Just my opinion though.

Agree most part. But I think it’s a tradeoff, especially in Python such a dynamic programming language. If we use other type checker, possibly the same things will happen again. Also, mypy is continuing improving, so I don’t pin a specific version and once the advanced features are added into mypy (like variable type re-definition), we can modify our code with minimal changes.
I think the most important part of type checker is to find the potential bugs, thus we need it. But we cannot notice the potential bugs under many type errors.

@Trinkle23897 Trinkle23897 marked this pull request as draft September 7, 2020 11:29
@Trinkle23897 Trinkle23897 changed the title mypy type check in unit test type check and docstring check in unit test Sep 7, 2020
@Trinkle23897 Trinkle23897 changed the title type check and docstring check in unit test type check in unit test Sep 11, 2020
@Trinkle23897 Trinkle23897 marked this pull request as ready for review September 11, 2020 08:49
@Trinkle23897 Trinkle23897 marked this pull request as draft September 12, 2020 01:27
Trinkle23897 added a commit that referenced this pull request Sep 12, 2020
Cherry-pick from #200 

- update the function signature
- format code-style
- move _compile into separate functions
- fix a bug in to_torch and to_numpy (Batch)
- remove None in action_range

In short, the code-format only contains function-signature style and `'` -> `"`. (pick up from [black](https://github.com/psf/black))
@Trinkle23897 Trinkle23897 marked this pull request as ready for review September 12, 2020 08:08
tianshou/__init__.py Outdated Show resolved Hide resolved
duburcqa
duburcqa previously approved these changes Sep 13, 2020
@Trinkle23897 Trinkle23897 merged commit b284ace into thu-ml:master Sep 13, 2020
@Trinkle23897 Trinkle23897 deleted the mypy branch September 13, 2020 11:31
BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
Cherry-pick from thu-ml#200 

- update the function signature
- format code-style
- move _compile into separate functions
- fix a bug in to_torch and to_numpy (Batch)
- remove None in action_range

In short, the code-format only contains function-signature style and `'` -> `"`. (pick up from [black](https://github.com/psf/black))
BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
Fix thu-ml#195: Add mypy test in .github/workflows/docs_and_lint.yml.

Also remove the out-of-the-date api
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.

Type check in the unit test
3 participants