fix type issues surfaced from pace integration#81
Conversation
| ntiw: int = DEFAULT_INT | ||
| ntcw: int = DEFAULT_INT | ||
| ntke: int = DEFAULT_INT |
There was a problem hiding this comment.
Those are already defined below with a comment (same default values)
| import datetime | ||
| import re | ||
| from pathlib import Path | ||
| from typing import Union |
There was a problem hiding this comment.
modern typing uses str | float instead of Union[str, float].
|
|
||
| def read_NOAA_solar_file( | ||
| solar_fname: Path, | ||
| ) -> dict[str : Union[float, int, dict[str : Union[int, float, dict[str:float]]]]]: |
There was a problem hiding this comment.
the format for typing dictionaries is dict[str, Any] not dict[str : Any]
| solar_constant_data["icy1"] = int(table_dat[2]) | ||
| solar_constant_data["icy2"] = int(table_dat[3]) | ||
| solar_constant_data["smean"] = float(table_dat[4]) | ||
| elif re.fullmatch(r"^(\*)\1{1,}$", table_dat[0]): |
There was a problem hiding this comment.
always use is (not) None to compare against None to help the type system.
| cfc11, | ||
| cfc12, | ||
| cfc22, | ||
| cfc113, |
There was a problem hiding this comment.
there was an inconsistency with those 14 or 15 return values. From usage, I inferred that the 15 one is probably what we want ... please double-check
There was a problem hiding this comment.
it looks like that's the missing one to me too based off of RTE_RRTMGPDriver in rte_rrtmgp.py.
There was a problem hiding this comment.
lol that's correct, whoops! Now I'm worried how that isn't causing issues currently
romanc
changed the title
The Quanity constructor will require a backend with the next version of NDSL.
|
Update: added a commit to add |
jjuyeonkim
left a comment
jjuyeonkim
left a comment
There was a problem hiding this comment.
It looks okay to me.
| cfc11, | ||
| cfc12, | ||
| cfc22, | ||
| cfc113, |
There was a problem hiding this comment.
it looks like that's the missing one to me too based off of RTE_RRTMGPDriver in rte_rrtmgp.py.
jjuyeonkim
left a comment
jjuyeonkim
left a comment
There was a problem hiding this comment.
Still looks okay to me. However, I'm going to wait on Oliver for this one.
|
@oelbert the NASA team would appreciate rapid feedback on this one: it is blocking (via NOAA-GFDL/pace#173) the release of NDSL version The proper fix (re-enabling |
| cfc11, | ||
| cfc12, | ||
| cfc22, | ||
| cfc113, |
There was a problem hiding this comment.
lol that's correct, whoops! Now I'm worried how that isn't causing issues currently
3 participants
Description
Continuing the quest towards the 2026.01.00 release, this PR fixes linting issues revealed in PR NOAA-GFDL/pace#173.
mypydidn't catch those inpySHiELDbecausemypywas effectively de-activated in the pySHiELD repo. The proper fix is a follow-up in #82. Here we just fix what's urgently needed to move the 2026.01.00 release of NDSL forward./cc @FlorianDeconinck
How Has This Been Tested?
Self-review of code, passing CI and I have
pace-linting working locally with these changes.Checklist: