AM5/ESM45 needed adjustments #690
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 85.32% 85.34% +0.02%
==========================================
Files 68 68
Lines 4490 4505 +15
==========================================
+ Hits 3831 3845 +14
- Misses 659 660 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ilaflott
force-pushed
the
am5-esm45-fixes
branch
from
bae7bb2 to
e3b1016
Compare
|
Not ready yet. There are some more netcdf attributes that need special handling. xarray loses metadata by default |
…" which is now users are writing it. Also, remove the _unmsk note from the error message so folks don't use it
.false. is now replacing False as the variable attribute trigger
fre/tests/test_files/reduced_ascii_files/atmos_cmip.ua_unmsk.cdl
Outdated
Show resolved
Hide resolved
This reverts commit 0fe44fd.
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
silent passing drives people (including me) nuts! maybe:
| except KeyError: | |
| pass | |
| except KeyError: | |
| fre_logger.warning( ... ) | |
| pass |
| try: | ||
| for static_source in component["static"]: | ||
| history_file = static_source["source"] | ||
| for i in range(1, ntiles+1): | ||
| dataset.to_netcdf(f"{input_dir}/{date}.{history_file}.tile{i}.nc") | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
try/except with a silent pass in a pytest set-up function seems ill-advised and likely to spawn silent problems. given that this is only run for testing, we should eschew try/except to cut down on the number of branching possibilities, and add an assert to enforce what this block is supposed to do for test setup
| for static_source_dict in pp_input_static_files: | ||
| outfile = output_subdir/f"{date}.{static_source_dict['source']}.nc" | ||
| test = xr.load_dataset(outfile) | ||
|
|
||
| assert "wet_c" not in test | ||
| assert "mister" in test | ||
| assert "darcy" in test | ||
| assert "wins" in test | ||
|
|
||
| assert np.all(test["mister"].values==np.float64(1.0)) | ||
| assert np.all(test["darcy"].values==np.float64(2.0)) | ||
| assert np.all(test["wins"].values==np.float64(3.0)) |
There was a problem hiding this comment.
how many static test cases are there? can we be more careful/explicit with checking the static case? We're overloading the variable test here... within a pytest test routine, at a different scope level. Gives me anxiety!
| files = glob.glob(target) | ||
| if len(files) >= 1: | ||
| fre_logger.debug("%s has %s files", target, len(files)) | ||
| input_files = glob.glob(source) |
There was a problem hiding this comment.
the term source is being used too often and too inconsistently. Here, source is really just a string input to glob, which uses it to find "source files".
Thus, if anything, input_files is actually the source's, not the input to glob
|
|
||
| fre_logger.info("Finished processing %s, wrote %s, pressure_mask is True", var, masked_var) | ||
| else: | ||
| fre_logger.debug("Not processing %s, no pressure_mask attr, nor _unmsk in the variable name", var) |
There was a problem hiding this comment.
why would we hide an error message that clearly explains the code's behavior?
Describe your changes
cdo merge.cdo mergedoes not work for more than 100 variables, and even with fewer variables inserts duplicate coordinate variables. It's possible there are slight metadata issues but the resulting climos work fine for analysis scriptsfre app mask-atmos-plevelneeds to trigger on pressure_mask =.false.instead ofFalseand update test case. Also, keep the _unmsk legacy behavior trigger, but remove it from the error message so users don't see it.Issue ticket number and link #677
Checklist before requesting a review