Fix mypy warnings for DiffusionConfig#1086
Conversation
|
cscs-ci run default |
|
cscs-ci run distributed |
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
| ValidDiffusionType = Literal[ | ||
| DiffusionType.NO_DIFFUSION, | ||
| DiffusionType.LINEAR_2ND_ORDER, | ||
| DiffusionType.SMAGORINSKY_NO_BACKGROUND, | ||
| DiffusionType.LINEAR_4TH_ORDER, | ||
| DiffusionType.SMAGORINSKY_4TH_ORDER, | ||
| ] | ||
| ValidSmagorinskyStencilType = Literal[ | ||
| SmagorinskyStencilType.DIAMOND_VERTICES, SmagorinskyStencilType.CELLS_AND_VERTICES | ||
| ] | ||
| ValidTemperatureDiscretizationType = Literal[ | ||
| TemperatureDiscretizationType.HOMOGENEOUS, TemperatureDiscretizationType.HETEROGENOUS | ||
| TemperatureDiscretizationType.HOMOGENEOUS, TemperatureDiscretizationType.HETEROGENEOUS | ||
| ] | ||
| ValidTurbulenceShearForcingType = Literal[ | ||
| TurbulenceShearForcingType.VERTICAL_OF_HORIZONTAL_WIND, | ||
| TurbulenceShearForcingType.VERTICAL_HORIZONTAL_OF_HORIZONTAL_WIND, | ||
| TurbulenceShearForcingType.VERTICAL_HORIZONTAL_OF_HORIZONTAL_VERTICAL_WIND, | ||
| TurbulenceShearForcingType.VERTICAL_HORIZONTAL_OF_HORIZONTAL_WIND_LTHESH, | ||
| ] |
There was a problem hiding this comment.
A bit off-topic, but why are those aliases needed?? They seem to be just duplicating the content of the Enums defined above, aren't they? If they are actually needed, I would add the TypeAlias hint (e.g. ValidDiffusionType: TypeAlias = Literal[...])
There was a problem hiding this comment.
@OngChia can you explain why those Literal aliases are needed?
There was a problem hiding this comment.
Sorry, it is my mistake. I did not notice they are all Enum not FrozenNamespace[int]
I was having an issue with mypy in my another PR#1075. mypy complained about wrong type
So, I decided to use Literal[...] for the type hint...
Related question: I am still confused about why mypy complains if I use
FrozenNamespace[int] instead of Enum and what is the correct usage of FrozenNamespace[int]
I have opened another small PR#1088 to remove these unnecessary Literal statements and fix further places where type_t_diffu and type_vn_diffu are stilled passed as int not TemperatureDiscretizationType and SmagorinskyStencilType. Why didn't the pre-commit git action catch them after I push my commit to my PR?
There was a problem hiding this comment.
Why didn't the pre-commit git action catch them after I push my commit to my PR?
I think this because we still exclude a lot of files from mypy checks...
Remove Valid** type Literal declaration for the config parameters `DiffusionType`, `SmagorinskyStencilType`, `TemperatureDiscretizationType`, and `TurbulenceShearForcingType` in DiffusionConfig after this [PR#1079](#1079) was merged. They are unnecessary because they are all Enum type (refer to comments in this [PR#1086](#1086)). Moreover, this PR fixes some additional places where `type_t_diffu` (`TemperatureDiscretizationType`) and `type_vn_diffu` (`SmagorinskyStencilType`) are still passed as int.
* main: Remove `vct_b` bindings only (#1082) Replace `pg_edgeidx_dsl` mask by multiplying with 0.0 (#1053) Replace `mask_hdiff` by multiplying with 0.0 (#1056) fix type hint in DiffusionConfig and remove Literal (#1088) muphys: graupel py2fgen bindings (#1073) Fix mypy warnings for DiffusionConfig (#1086) Add tracer advection to standalone driver [cycle 34] (#1039) Reject namelist parameters not supported (#1079) Update to gt4py v1.1.6 (#1081) Merge bdy halo stencils (#1066) Enable gauss3d experiment for distributed tests (#1013) Reenable MPS in CI (#1057) Don't compute RBF coefficients over whole domain to avoid non-positive definite matrices (#1077) muphys: Only write the surface level of the fluxes (#995) Update to gt4py v1.1.5 (#1072)
4 participants
#1079 introduced stricter type checks for DiffusionConfig parameters. Two parameters were still being passed as plain ints which mypy complained about.
I've also renamed HETEROGENOUS to HETEROGENEOUS (added an E) for consistency with HOMOGENEOUS. Also see https://en.wikipedia.org/wiki/Homogeneity_and_heterogeneity#Etymology_and_spelling.