Fix convert_tdas_to_huss guard: submissions with 'ps' but no 'tdas' raise KeyError - #22
Open
eastagiletracker wants to merge 1 commit into
Open
eastagiletracker wants to merge 1 commit into
eastagiletracker wants to merge 1 commit into
Conversation
`if 'tdas' and 'ps' in ds.data_vars` evaluates as `'tdas' and ('ps' in
ds.data_vars)`, so the 'tdas' membership was never tested. A submission
carrying 'ps' but neither 'tdas' nor 'huss' entered the conversion branch
and raised KeyError instead of falling through to the "No 'tdas' or 'ps'
available to calculate 'huss'." message.
Adds unit tests covering the three branches of convert_tdas_to_huss and
the compute_huss_from_tdas conversion, a `make test` target that runs
them, and pytest in the evaluation environment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes a fix to the
huss/tdasguard inevaluations/notebooks/aimip_data_utils.py, where the'tdas'membership is never actually tested, so a submission that carriespsbut neithertdasnorhussraisesKeyErrorin the middle of every E1–E5 run instead of taking the fallback branch that was written for it. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/227. You can sign in with your GitHub ID to claim ownership of the project.What is wrong
convert_tdas_to_hussopens withPython parses that as
'tdas' and ('ps' in ds.data_vars) and ('huss' not in ds.data_vars)— the non-empty string'tdas'is simply truthy, so whether the dataset actually containstdasis never checked. A dataset withps, notdasand nohusstherefore enters the conversion branch, printsConverting 'tdas' to 'huss'., and dies onds['tdas']. Theelifbranch right below it —No 'tdas' or 'ps' available to calculate 'huss'.— is unreachable for that case, even though it is exactly the case it describes.That matters because all five evaluation notebooks apply this function to every submission they loaded, in the
Checking {experiment_submission_name} for huss/tdas.loop (E5 runs it three times, once per experiment, and most of the notebooks also call it on the ERA5 dataset), andSUBMISSIONS.mdalready records that coverage differs per model — DLESyM is noted as having "only a small subset of variables available", and cBottle deriveshussfrom surface pressure and dewpoint. A submission that reportspsbut neither humidity variable aborts the notebook cell rather than being reported as missing and skipped.Reproduction at current
main(d376068)From
evaluations/notebooks, in theenvironment.ymlenvironment:Driving it the way the notebooks do, with three submissions (one with
tdas+ps, one withhuss, one withpsonly), the loop stops at the third:The change
One condition:
if 'tdas' in ds.data_vars and 'ps' in ds.data_vars and 'huss' not in ds.data_vars:. Every input that works today takes the same branch as before and produces the same values — only the previously-crashing case moves, fromKeyErrorto the message the function already had for it. After the fix, the same loop finishes:Verification
evaluations/notebooks/test_aimip_data_utils.pyis new and covers the three branches ofconvert_tdas_to_huss(converts whentdas+psare present and dropstdas; leaves a dataset withouttdasalone; keeps a submittedhussand leavestdasin place), thetdas-without-psedge, and thecompute_huss_from_tdasconversion itself against an independent value (280 K dewpoint at 1000 hPa is about 6.19 g/kg). Run againstmainunchanged,test_convert_tdas_to_huss_without_tdasfails with theKeyErrorabove and the other four pass; with the one-line change all five pass:The repository's existing check is unaffected —
make test-envprintsAll imports OKbefore and after, and no notebook, cached dataset or figure is touched.make testis added as a sibling target andpytesttoenvironment.ymlso the new test is runnable in the documented environment (make envneeds re-running once to pick it up); happy to wire that target into a CI workflow in a follow-up if you would like one.How this was managed
This work was tracked as a single story, Fix convert_tdas_to_huss guard, on a board at https://eastagiletracker.com/projects/227 built by importing this repository's own history — 21 stories from your 3 issues and 18 pull requests, with their labels.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com