Activate mypy in ignite.metrics by gruebel · Pull Request #1391 · pytorch/ignite

gruebel · 2020-10-18T21:51:48Z

Description:
That was some work, I hope I made everything correct. Sometimes I had to rearrange the code, so mypy understood the typing better.

vfdev-5

Thanks a lot @gruebel ! Great work on the typing and mypy.
I left few comments. I have to understand why for mypy we have to define arguments in the constructor (which calls reset) as inside reset to avoid temporary None assignement.

ignite/metrics/accumulation.py

ignite/metrics/confusion_matrix.py

ignite/metrics/metric.py

ignite/metrics/accumulation.py

ignite/metrics/accuracy.py

vfdev-5

@gruebel thanks a lot for working on this PR !
Reviewing the PR, I see that we previously mixed up with compute output type. I think the main point is to have it as float everywhere it is possible and torch.Tensor otherwise or other structures if intended.
I left some comments in the code to update the current code. I didn't manage to review all 17 files. Review others later.

ignite/metrics/accumulation.py

ignite/metrics/confusion_matrix.py

ignite/metrics/metric.py

ignite/metrics/precision.py

vfdev-5

Thanks a lot the update Anton ! I think we are almost on the finish line. I left some comments to tweak few things and it is good to go.

ignite/metrics/accumulation.py

ignite/metrics/confusion_matrix.py

ignite/metrics/frequency.py

vfdev-5 · 2020-10-26T22:21:14Z

ignite/metrics/precision.py

-        self._true_positives = None
-        self._positives = None
        self.eps = 1e-20
+        self._is_reduced = False


Normally, self._is_reduced = False is called in Metric.__init__, so let's remove it from here.

if I remove it, then I get a mypy issue and have to ignore it like I did before

if not self._is_reduced: # type: ignore[has-type] # mypy can't determine type

Yeah, I do not understand why it looses the type. I checked with reveal_type(self._is_reduced) inside _BaseClassification and Metric and it is bool. But in Precision it becomes Any...

what do you prefer? Define it in the __init__ or add the ignore?

OK, let's add ignore for this PR and fix if possible after.

I think I found how to fix it:

if not self._is_reduced: self._true_positives = idist.all_reduce(self._true_positives) # type: ignore[arg-type, assignment] self._positives = idist.all_reduce(self._positives) # type: ignore[arg-type, assignment] self._is_reduced = True # type: bool

while removing unnecessary initialization in the constructor self._is_reduced = False.

you are right and it works, but I have no idea, why this fixes this problem?!

ignite/metrics/precision.py

vfdev-5 · 2020-10-26T22:24:46Z

ignite/metrics/recall.py

            if not self._average:
-                self._true_positives = torch.cat([self._true_positives, true_positives], dim=0)
-                self._positives = torch.cat([self._positives, actual_positives], dim=0)
+                self._true_positives = torch.cat(


Here it should be almost the same as for precision (L180-L181), right ?

self._true_positives = torch.cat([cast(torch.Tensor, self._true_positives), true_positives], dim=0) self._positives = torch.cat([cast(torch.Tensor, self._positives), all_positives], dim=0)

yeah, you are right, but it is so strange. My IDE is totally fine with using cast, but mypy still complains with

error: Cannot determine type of '_true_positives' [has-type] error: Cannot determine type of '_positives' [has-type]

I even tried the newer mypy version, still same error 😢

vfdev-5 · 2020-10-26T22:28:17Z

ignite/metrics/ssim.py

    @reinit__is_reduced
    def reset(self) -> None:
-        self._sum_of_batchwise_ssim = 0.0  # Not a tensor because batch size is not known in advance.
+        self._sum_of_batchwise_ssim = (


Now, self._sum_of_batchwise_ssim is defined as a tuple, right ? Can't we keep it as float ?

yeah, it is still float, but black just formatted it. To be a single value tuple it needs to be written with a comma

self._sum_of_batchwise_ssim = (0.0, )

I will move the comment just above the attribute assignment

I mean, previously it was a plain float, not tuple and now it becomes a tuple with a single float:

- self._sum_of_batchwise_ssim = 0.0 + self._sum_of_batchwise_ssim = (0.0, )

It is not intended, right ?

sorry, what I meant
0.0 = (0.0) != (0.0, )
So it is still a float and not a tuple 😄

Thanks ! Never got that 0.0 = (0.0) :)

I have seen it many times, when I started to use black. I usually used that syntax for multiline strings.

vfdev-5

Thanks a lot, Anton !
Really huge work was done here ! LGTM

* Update TQDM to > 4.48.0 (pytorch#1339) * Fix tqdm 4.49.0 tests. * update requirements. * Use distutils instead of packaging for getting package version. * Reformat code. * Reformat using black 19.10b0. Co-authored-by: vfdev <[email protected]> * Activate mypy in ignite.utils (pytorch#1352) * Activate mypy in ignite.utils * Add missing torch package to lint stage * Move mypy check to test stage Co-authored-by: vfdev <[email protected]> * Exposed EventEnum in docs and added import statement in its examples (pytorch#1345) (pytorch#1353) * Update Shpinx to v3.1. (pytorch#1356) Fixes pytorch#1272 * Update README.md * Update doc of examples for Trains fileserver setup (pytorch#1360) * update doc for trains fileserver setup * replace github issue by documentation Co-authored-by: Desroziers <[email protected]> * Updated commit fix (pytorch#1361) (pytorch#1364) * Updated commit fix * Update code-style.yml Co-authored-by: rex_divakar <[email protected]> * added tuple type to mixins.py (pytorch#1365) * added tuple type to mixins.py * allow mypy to pass through base file * fixed linting issues Co-authored-by: vfdev <[email protected]> * Activate mypy in ignite.distributed (pytorch#1355) * Activate mypy in ignite.distributed * Fix tests & py3.5 inline type hints * Remove typing,overload * Fix multiple typing issues * Fix typing issues * Fix TPU test Co-authored-by: vfdev <[email protected]> * Improve typing for ignite.handlers module (1343) (pytorch#1349) * Improve typing for ignite.handlers module (1343) * autopep8 fix * Fix typing for py35, remove handlers block from mypy.ini * Add exception to ModelCheckpoint when saving last checkpoint * Add test for ModelCheckpoint with redefined save_handler case * autopep8 fix Co-authored-by: AutoPEP8 <> Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> Co-authored-by: trsvchn <[email protected]> * [3] [contrib/metrics] setup typing in contrib part of the library (pytorch#1363) * [3] [contrib/metrics] setup typing in contrib part of the library * review changes * Update gpu_info.py Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> * [2] [contrib/handlers] setup typing in contrib part of the library (pytorch#1362) * [2] [contrib/handlers] setup typing in contrib part of the library * Fix a typo in tqdm logger * review comments * Update mlflow_logger.py * Update neptune_logger.py * review changes * review changes Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> * [1] [contrib/engines] setup typing in contrib part of the library (pytorch#1351) * setup typing for contribute/engine part of the code * revert doc string changes * Update common.py Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> * Update PULL_REQUEST_TEMPLATE.md * Disable cross-ref links for type annotations (pytorch#1374) * Added reinit__is_reduced and sync_all_reduce docs in metrics doc (pytorch#1373) * added links to reinit__is_reduced and sync_all_reduce decorators in metrics documentation * updated order in list of metrics * deleted decorators from metric list * Update metrics.rst Co-authored-by: vfdev <[email protected]> * warning if current device index is lower than current local rank (pytorch#1335) (pytorch#1376) * warning if current device index is lower than current local rank (pytorch#1335) * warning if current device index is lower than current local rank * Updated code and tests * Fixed formatting * Updated code and tests for horovod - fixed failing test * Updated tests Co-authored-by: vfdev-5 <[email protected]> * Removed debug prints * Fixed failing hvd tests Co-authored-by: Sai Sandeep Mutyala <[email protected]> * Github Actions workflow CI for horovod on CPU (pytorch#1377) * Initial commit * Update hvd-tests.yml * Update hvd-tests.yml * Update hvd-tests.yml * trigger GitHub actions * removed examples * trigger GitHub actions * Improve typing of distributed.comp_modules.utils.all_gather (pytorch#1370) * Improve typing of distributed.comp_modules.utils.all_gather * Fix all_gather gloo test * Fix XLA test Co-authored-by: vfdev <[email protected]> * Removed state.restart method (pytorch#1385) * Activate mypy in ignite.engine (pytorch#1379) * Activate mypy in ignite.engine * Fix missing import * Fix typing issues with nighty build * Fix PR findings Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> * add acknowledgment for IFPEN (pytorch#1387) Co-authored-by: Desroziers <[email protected]> * Fix collections DeprecationWarning (pytorch#1388) * Remove deprecated CustomPeriodicEvent from nb example, fix tb OutputHadler (pytorch#1389) * Update checkpoint.py (pytorch#1394) * Add GH Action to build and publish Docker images (pytorch#1390) * ADD GH Action to build and publish Docker images * Configure GH action for Docker build * Fix image_tag fetching * Fix identation for main steps * Add token envs for GH docker action, fix push all tag_image * Switch to horovod 0.20.3 * Push images on push events * Fix if conditional * Toctrees for classes and methods using sphinx autosummary (pytorch#1393) * Implement autosummary patch for autolisting * Fix css for autogenerated tables via autosummary * Improve autolist feature * Add toctrees for methods and classes for ignite * Better import for autolist * Add toctrees for methods and classes for contrib package * Fix CSS for autosummary table row height * Fix warnings raised by toctree * Remove all deprecated args, kwargs for v0.5.0 (pytorch#1396) (pytorch#1397) * Remove all deprecated args, kwargs for v0.5.0 (pytorch#1396) * Improve deprecation message of setup_any_logging (pytorch#1396) * Update setup.cfg * Remove module members w/o docstrings from autolist toc tables (pytorch#1401) * Add --color=yes in pytest config (pytorch#1402) * removes styling of function descriptions as requested in pytorch#1256 (pytorch#1399) * removes styling of function descriptions as requested in pytorch#1256 * reverts modifications to the example files * Skip circle ci and GA builds (pytorch#1400) * [WIP] Skip circle ci and GA builds * Fixes swissknife version * Replaced swissknife by sh script * [skip ci] Updated trigger_if_modified.sh script - excluded docs from github actions - added 1.6.0 to pytorch-version-tests * Fixes pytorch#1408, XLA failing tests (pytorch#1412) - Issue is related to xla nightly - Probably related to pytorch/xla#2576 * Added Mypy check as github action (pytorch#1418) * Added Mypy check to as github action * Removed py3.5 * Activate mypy in ignite.metrics (pytorch#1391) * Activate mypy in ignite.metrics * remove num_examples check * fix CR issues * remove init defaults in Accuracy * Remove None assignments in __init__ * improved typing connected with mypy issues Co-authored-by: vfdev <[email protected]> * Update README.md Badge travis org -> com * add tolerance for tpu in r2 and canberra tests (pytorch#1414) * add tolerance for tpu * autopep8 fix * Reverted test_canberra_metric.py as unnecessary * Update test_r2_score.py * Update test_r2_score.py Co-authored-by: Desroziers <[email protected]> Co-authored-by: sdesrozis <[email protected]> Co-authored-by: vfdev <[email protected]> * Activate mypy ignite contrib metrics (pytorch#1419) * Activate mypy in ignite.contrib.metrics * Add missing type hints in ignite.contrib.metrics * Add missing type hints * Contributing guide (pytorch#1424) * Add materials on how to setup dev env in CONTRIBUTING guide pytorch#1395 - first draft of first-time contributor guide * Add materials on how to setup dev env in CONTRIBUTING guide pytorch#1395 - add in Table of Contents * Add materials on how to setup dev env in CONTRIBUTING guide pytorch#1395 - fix table of contents link * Add materials on how to setup dev env in CONTRIBUTING guide pytorch#1395 - rollback README.md, remove IDE setting * Update CONTRIBUTING.md Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> * replace Number type with float; remove unneeded type ignores (pytorch#1425) Co-authored-by: vfdev <[email protected]> * Update README.md * Added new time profiler `HandlersTimeProfiler` which allows per handler time profiling (pytorch#1398) * added new HandlersTimeProfiler with handler level details and added tests for HandlersTimeProfiler (pytorch#1346) * updated docs and docstring for HandlersTimeProfiler (pytorch#1346) * updated HandlersTimeProfiler to support any events and updated detach mechanism of profiler (pytorch#1346) * updated HandlersTimeProfiler with code improvements and implemented csv export method (pytorch#1346) * updated HandlersTimeProfiler to handle event handler bundle better (pytorch#1346) * HandlersTimeProfiler: added threshold for filtering profiled time for handlers attached to event with filters (pytorch#1346) * HandlersTimeProfiler: add tests and type hints (pytorch#1398) * HandlersTimeProfiler: use FloatTensor for list to tensor conversion (pytorch#1398) * HandlersTimeProfiler: use torch.tensor for list to tensor conversion (pytorch#1398) * HandlersTimeProfiler: remove unnecessary import (pytorch#1398) * HandlersTimeProfiler: move tensor conversion in compute_basic_stats (pytorch#1398) Co-authored-by: vfdev <[email protected]> * Fix HandlersTimeProfiler example rendering (pytorch#1428) * Fix HandlersTimeProfiler example rendering * Fix WARNINGs: Title underline too short * Add horizontal scrollbars for examples instead of font-size tweaks * Enable horizontal scrollbars for examples globally * Save model with same filename (pytorch#1423) * save model with same filename * Update checkpoint.py * use elif * refactor to have only one comprehension list * refactoring * improve test * autopep8 fix Co-authored-by: Desroziers <[email protected]> Co-authored-by: vfdev <[email protected]> Co-authored-by: sdesrozis <[email protected]> * Some docs nit picking (pytorch#1435) * enable extra flags for stricter type checking (pytorch#1433) * Fixes pytorch#1426 - distrib cpu tests on win (pytorch#1429) * Fixes pytorch#1426 - distrib cpu tests on win * Skip distributed/tpu/multinode_distributed if SKIP_DISTRIB_TESTS=1 * replaced sh by bash * Update pytorch-version-tests.yml * Updated files to ignore for GitHub Actions CI (pytorch#1441) * Added windows gpu test to circle ci (pytorch#1440) * [WIP] Added windows gpu test to circle ci * Updated windows config * Updated conda installation * Updated miniconda install command * Removed conda installation * Updated configuration * Adding max_iters as an optional arg in Engine run (pytorch#1381) * initial draft, adding max_iters as optional args in run * fixed typo * minor bug fixes * resolving failing tests * fixed out-of-place conditional * typo fix * updated docstring for 'run' * added initial tests * (WIP) restructured creating a new state with max_iters * updated tests & docstrings * initial draft, adding max_iters as optional args in run * fixed typo * minor bug fixes * resolving failing tests * fixed out-of-place conditional * typo fix * updated docstring for 'run' * added initial tests * (WIP) restructured creating a new state with max_iters * updated tests & docstrings * added test to check _is_done * updating engine loop condition * autopep8 fix * linting issues * fixed mypy errors * fixed formatting * minor fix & add test for larger max_iters * removed unused typechecking Co-authored-by: thescripted <[email protected]> Co-authored-by: vfdev <[email protected]> * Updated circleci trigger_if_modified.sh if on master * Fix failing distributed ci (pytorch#1445) * Setup random free port for distrib ci * autopep8 fix Co-authored-by: vfdev-5 <[email protected]> * Added torch.cuda.manual_seed_all(seed) (pytorch#1444) * fix ReduceOp typing issue (pytorch#1447) * Update README.md * Updated miniconda setup (pytorch#1449) * Fixed broken coverage (pytorch#1451) * Fixed broken coverage * Updated hvd-tests.yml * Migrated nightly build/release to GitHub Actions (pytorch#1448) (pytorch#1450) * Added nightly build/release action * Updated yml * Updated to conda-incubator/setup-miniconda@v2 * Reverted modifications in other actions * Migrated nightly build/release to GitHub Actions (pytorch#1448) * Added nightly build/release action * Updated yml * Updated to conda-incubator/setup-miniconda@v2 * Reverted modifications in other actions * Fix PyPi upload * Finalized binaries-nightly-release.yml * Updated README * [contributing] add syncing up with the upstream (pytorch#1452) * Update setup.cfg * [contributing] add syncing up with the upstream * Apply suggestions from code review Co-authored-by: vfdev <[email protected]> * Update CONTRIBUTING.md Co-authored-by: vfdev <[email protected]> * [ci] create docs.yml * install torch * Activate MyPy in ignite.contrib.engines (pytorch#1416) * Activate mypy in ignite.contrib.engines * Fix review comments * fix extra event too * Update to fix strict errors * Update quickstart.rst (pytorch#1460) * Update quickstart.rst Plz have a look if I am going correct or not. ###rewording sentences to simplify the understanding * Update docs/source/quickstart.rst Co-authored-by: vfdev <[email protected]> * Update quickstart.rst * Update quickstart.rst * Update docs/source/quickstart.rst Co-authored-by: vfdev <[email protected]> * Update quickstart.rst Final commit is done. You can review it. Co-authored-by: vfdev <[email protected]> * [docs] intersphinx update in conf.py * [docs] add missing function in handlers docs (pytorch#1463) * Update setup.cfg * [docs] missing function in handlers docs * [docs] add ignite favicon (pytorch#1462) * Update setup.cfg * [docs] add ignite favicon * Add missing classes and links for docs (pytorch#1461) * Update CONTRIBUTING.md * Update concepts.rst (pytorch#1465) * setup toml, yaml, prettier in pre-commit (pytorch#1468) * Update setup.cfg * [pre-commit] setup yaml in pre-commit hook * [pre-commit] setup toml prettier * [docs] make GIF look good on mobile (pytorch#1470) * Update setup.cfg * [docs] make gif fit on mobile * Update index.rst * use .. raw:: html * Update index.rst Co-authored-by: vfdev <[email protected]> * Update README.md * Update README.md * Update README.md * Update README.md * [docs] add submodule in engine.rst (pytorch#1464) * Update setup.cfg * [docs] add submodule in engine * [docs] add suggestions, contrib engine docs and 45% width * Update ignite_theme.css * [metrics] speed up SSIM tests (pytorch#1467) * Update setup.cfg * [metrics] update ssim * use np.allclose instead of torch.allclose * Apply suggestions from code review * extract into _test_ssim * extract into scripts * fix path * fix path * fix path * good to go! * [ci] universal conda build (pytorch#1471) * Update setup.cfg * rm conda_build_config * rm conda_build_config * Update docs.yml * Update install_docs_deps.sh Co-authored-by: vcarpani <[email protected]> Co-authored-by: Anton Grübel <[email protected]> Co-authored-by: Harsh Patel <[email protected]> Co-authored-by: Théo Dumont <[email protected]> Co-authored-by: Sylvain Desroziers <[email protected]> Co-authored-by: Desroziers <[email protected]> Co-authored-by: rex_divakar <[email protected]> Co-authored-by: Benjamin Kinga <[email protected]> Co-authored-by: Taras Savchyn <[email protected]> Co-authored-by: trsvchn <[email protected]> Co-authored-by: RaviTeja Pothana <[email protected]> Co-authored-by: Josseline Perdomo <[email protected]> Co-authored-by: Sai Sandeep Mutyala <[email protected]> Co-authored-by: Ramesht Shukla <[email protected]> Co-authored-by: botbotbot <[email protected]> Co-authored-by: Jeff Yang <[email protected]> Co-authored-by: Sergey Epifanov <[email protected]> Co-authored-by: sdesrozis <[email protected]> Co-authored-by: zhxxn <[email protected]> Co-authored-by: François COKELAER <[email protected]> Co-authored-by: thescripted <[email protected]> Co-authored-by: vfdev-5 <[email protected]> Co-authored-by: Rostyslav Zatserkovnyi <[email protected]> Co-authored-by: Afzal <[email protected]>

Activate mypy in ignite.metrics

c8ad7dc

vfdev-5 reviewed Oct 18, 2020

View reviewed changes

ignite/metrics/accumulation.py Outdated Show resolved Hide resolved

ignite/metrics/accumulation.py Outdated Show resolved Hide resolved

ignite/metrics/confusion_matrix.py Outdated Show resolved Hide resolved

ignite/metrics/metric.py Show resolved Hide resolved

remove num_examples check

7164163

vfdev-5 reviewed Oct 22, 2020

View reviewed changes

ignite/metrics/accumulation.py Outdated Show resolved Hide resolved

vfdev-5 reviewed Oct 22, 2020

View reviewed changes

ignite/metrics/accuracy.py Outdated Show resolved Hide resolved

gruebel added 2 commits October 24, 2020 09:45

fix CR issues

8287e6f

remove init defaults in Accuracy

bcac5c5

vfdev-5 reviewed Oct 25, 2020

View reviewed changes

vfdev-5 mentioned this pull request Oct 25, 2020

Fix incorrect design for RunningAverage, Frequency, ModelCheckpoint, ... #1405

Closed

Remove None assignments in __init__

caa99f5

gruebel mentioned this pull request Oct 26, 2020

Replace type hint numbers.Number with float #1407

Closed

vfdev-5 reviewed Oct 26, 2020

View reviewed changes

improved typing connected with mypy issues

60eef40

vfdev-5 approved these changes Oct 28, 2020

View reviewed changes

Merge branch 'master' into activate-mypy-in-ignite-metrics

291008e

vfdev-5 merged commit 9b57459 into pytorch:master Oct 28, 2020

gruebel deleted the activate-mypy-in-ignite-metrics branch October 31, 2020 09:54

Uh oh!

Conversation

gruebel commented Oct 18, 2020

Uh oh!

vfdev-5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vfdev-5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vfdev-5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vfdev-5 Oct 27, 2020 •

edited

Loading

vfdev-5 Oct 27, 2020 •

edited

Loading