type-checking investigation #68

mathematicalmichael · 2022-11-12T21:06:15Z

https://github.com/mathematicalmichael/mud/actions/runs/3425141460/jobs/5705619794 failed in a way I can't seem to reproduce locally.

mypy src/mud tests works fine for me, but perhaps there's a version difference in the github action that can explain things.

In the runner:

Python 3.10.8
mypy 0.990
...
Found 32 errors in 7 files (checked 24 source files)

In my env:

Python 3.9.7
mypy 0.982
...
Found 0 errors

upgrading to mypy==0.990 reproduces the error. This is a recent release.

of the 32 errors, 10 of them are implicit optional for save_path
- currently save_figure acts more as "maybe save figure" and that's confusing, so while it's inefficient to do so (rather than renaming the function to include an awkward prepended maybe_), I'm updating the logic for whether to call this function to the calling function. i.e. I'm adding a bunch of if save_path: indent blocks, this avoids changing the defaults /docstrings and allows for the option to use the type hint Optional[str].
- one function had two unused arguments, so I removed them
- refactored functions to reduce "complexity" according to flake8.
of the remaining 22 errors... all fixed automatically with the no_implicit_optional tool.

cdelcastillo21

Just is not None: checks according to PEP 8. I think I marked them all.

cdelcastillo21 · 2022-11-15T16:43:19Z

src/mud/examples/adcirc.py

-    save_figure(
-        f"si-{value}", save_path, close_fig=close_fig, dpi=dpi, bbox_inches="tight"
-    )
+    if save_path:


Should be
if save_path is not None:
See PEP 8

hm, why doesn't flake8 complain about this?
my thinking was actually to check for both None and "" simultaneously.
Though perhaps the behavior of "" is fine... realized I'm not actually sure what would happen (would it try to save to root or to cwd?)

Also, beware of writing if x when you really mean if x is not None – e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

the type is str, and I did intend for "" to evaluate in the same context (this is the only str that evals to False), so I was "careful" in the sense that this recommendation suggests. Question is... was I right about my assumption that "" is a bad argument to pass to save_figure? I don't know what happens if you try yet. I would prefer "cwd" to be given by "." which evals to True, but I can see an argument for both strings resulting in that behavior of saving to cwd.

Either works. I think PEP8 is just there to make the code more clear and readable in this case. "" evaluates to None but if not explicitly stated it just obfuscates the code a bit.

fwiw, i ended up doing this.

cdelcastillo21 · 2022-11-15T16:43:54Z

src/mud/examples/adcirc.py

-    save_figure(
-        "adcirc_full_ts", save_path, close_fig=close_fig, dpi=dpi, bbox_inches="tight"
-    )
+    if save_path:


Should be
if save_path is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:44:00Z

src/mud/examples/adcirc.py

-            dpi=dpi,
-            bbox_inches="tight",
-        )
+        if save_path:


Should be
if save_path is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:44:33Z

src/mud/examples/adcirc.py

-            dpi=dpi,
-            bbox_inches="tight",
-        )
+        if save_path:


Should be
if save_path is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:44:37Z

src/mud/examples/adcirc.py

-            dpi=dpi,
-            bbox_inches="tight",
-        )
+        if save_path:


Should be
if save_path is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:48:15Z

src/mud/examples/poisson.py

+        mud_prob.plot_params_2d(ax=ax, y=i, contours=True, colorbar=True)
+        if i == 1:
+            ax.set_ylabel("")
+    if kwargs.get("save_path"):


Should be
if kwargs,get("save_path") is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:48:24Z

src/mud/examples/poisson.py

+    )
+
+    fig.tight_layout()
+    if kwargs.get("save_path"):


Should be
if kwargs,get("save_path") is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:48:46Z

src/mud/examples/poisson.py

-        save_figure(
-            "lam1", save_path, close_fig=close_fig, dpi=dpi, bbox_inches="tight"
-        )
+        if save_path:


Should be
if save_path is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:49:05Z

src/mud/examples/poisson.py

-        save_figure(
-            "lam2", save_path, close_fig=close_fig, dpi=dpi, bbox_inches="tight"
-        )
+        if save_path:


Should be
if save_path is not None:
See PEP 8

cdelcastillo21 · 2022-11-15T16:49:12Z

src/mud/examples/poisson.py

-            dpi=dpi,
-            bbox_inches="tight",
-        )
+        if save_path:


Should be
if save_path is not None:
See PEP 8

* switch to ini file * save figs to test_dir * Revert "switch to ini file" This reverts commit 597fccc. * linting * workflows should trigger for changes to tests * specify dependency ranges (#70) * specify dependency ranges I ended up trying to install mud on a computer with an older scikit-learn, and it ended up using the cache of like version 0.something, so I want to at least specify the major version we require for the more important libraries. * loosen matplotlib * 3.7 support is a pain... * typo * prevent warnings from numpy * specify build os, tools * fix flake8 E721 errors * [black] lint files * [black] notebook linting * cover more versions of python * type-checking investigation (#68) * print mypy and python versions in workflow * 'if save_path' used to fix 10 mypy errors * automatic fix using https://github.com/hauntsaninja/no_implicit_optional * post-fix linting * refactor to address C901: too complex * refactor to address C901: too complex * ignore C901 * fix typo * sort imports * if save_path is not None * no more 3.12 bc numpy 1.24 * fix all mypy issues with 1.8.0 * enforce old cli functionality * actions updates, experiments with 3.12 (#72) * experiment with 3.12 * loosen pandas for 3.12 * build updates * rename, try new build

mathematicalmichael added 9 commits November 12, 2022 21:05

print mypy and python versions in workflow

237f848

'if save_path' used to fix 10 mypy errors

8f563f1

automatic fix using https://github.com/hauntsaninja/no_implicit_optional

313104f

post-fix linting

50a17f9

refactor to address C901: too complex

337feef

refactor to address C901: too complex

48ec699

ignore C901

5ea6f5c

fix typo

662fcea

sort imports

f1d3d4e

mathematicalmichael requested a review from cdelcastillo21 November 12, 2022 23:27

mathematicalmichael mentioned this pull request Nov 13, 2022

save images during test to .test_dir/ #69

Merged

cdelcastillo21 requested changes Nov 15, 2022

View reviewed changes

mathematicalmichael force-pushed the dev branch from 46febc3 to 347ffaa Compare January 22, 2024 04:04

mathematicalmichael added 4 commits January 22, 2024 04:27

Merge branch 'dev' into feature/type-checking

beb0587

if save_path is not None

82c5da1

no more 3.12 bc numpy 1.24

4e22421

fix all mypy issues with 1.8.0

7de8936

mathematicalmichael merged commit 94084e7 into dev Jan 22, 2024
13 checks passed

mathematicalmichael deleted the feature/type-checking branch January 22, 2024 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

type-checking investigation #68

type-checking investigation #68

mathematicalmichael commented Nov 12, 2022 •

edited

Loading

cdelcastillo21 left a comment

cdelcastillo21 Nov 15, 2022

mathematicalmichael Nov 15, 2022 •

edited

Loading

mathematicalmichael Nov 15, 2022 •

edited

Loading

cdelcastillo21 Dec 20, 2022

mathematicalmichael Jan 22, 2024

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

cdelcastillo21 Nov 15, 2022

type-checking investigation #68

type-checking investigation #68

Conversation

mathematicalmichael commented Nov 12, 2022 • edited Loading

cdelcastillo21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathematicalmichael Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

mathematicalmichael Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathematicalmichael commented Nov 12, 2022 •

edited

Loading

mathematicalmichael Nov 15, 2022 •

edited

Loading

mathematicalmichael Nov 15, 2022 •

edited

Loading