Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: Silent bug in adjust_prs_val_units() conditional where if prs_val0: will be False if prs_val0 is 0 #797

Open
tomvothecoder opened this issue Mar 6, 2024 · 1 comment
Open

[Bug]: Silent bug in adjust_prs_val_units() conditional where if prs_val0: will be False if prs_val0 is 0 #797

tomvothecoder opened this issue Mar 6, 2024 · 1 comment
Labels
bug Bug fix (will increment patch version)

Comments

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Mar 6, 2024 •
edited
Loading

What happened?

Overview

There is a conditional in adjust_prs_val_units() that checks if prs_val0: to perform unit adjustment on the prs axis. This is a silent bug in this conditional where unit adjustment is not being performed when prs_val0 == 0 because 0 == False in boolean form.

Related code:

e3sm_diags/e3sm_diags/derivations/acme.py

Lines 433 to 446 in 43f5f74

def adjust_prs_val_units(
prs: "FileAxis", prs_val: float, prs_val0: Optional[float]
) -> float:
"""Adjust the prs_val units based on the prs.id"""
# COSP v2 cosp_pr in units Pa instead of hPa as in v1
# COSP v2 cosp_htmisr in units m instead of km as in v1
adjust_ids = {"cosp_prs": 100, "cosp_htmisr": 1000}
if prs_val0:
prs_val = prs_val0
if prs.id in adjust_ids.keys() and max(prs.getData()) > 1000:
prs_val = prs_val * adjust_ids[prs.id]
return prs_val

Example Case

There are several derived variables that define prs_low0=0. For example, notice how CLDLOW_TAU_1.3_9.4_MISR has a prs_low0=0 below. For the dataset I was working with, this value is never used due to the silent bug. No prs axis adjustments occur and the prs low value from the dataset (-0.001) is used which results in incorrect results.

e3sm_diags/e3sm_diags/derivations/acme.py

Lines 1378 to 1393 in 43f5f74

"CLDLOW_TAU1.3_9.4_MISR": OrderedDict(
[
(
("CLD_MISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 1.3, 9.4), target_units="%"
),
),
(
("CLMISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 1.3, 9.4), target_units="%"
),
),
]
),

What did you expect to happen? Are there are possible answers you came across?

The conditional should be updated to if prs_val0 is not None: since prs_val0 is optional. This logic is implemented correctly on the dev branch, cdat-migration-fy24.

def adjust_prs_val_units(
    prs: "FileAxis", prs_val: float, prs_val0: Optional[float]
) -> float:
    """Adjust the prs_val units based on the prs.id"""
    # COSP v2 cosp_pr in units Pa instead of hPa as in v1
    # COSP v2 cosp_htmisr in units m instead of km as in v1
    adjust_ids = {"cosp_prs": 100, "cosp_htmisr": 1000}


    if prs_val0 is not None:
        prs_val = prs_val0
        if prs.id in adjust_ids.keys() and max(prs.getData()) > 1000:
            prs_val = prs_val * adjust_ids[prs.id]


    return prs_val

Minimal Complete Verifiable Example (MVCE)

No response

Relevant log output

No response

Anything else we need to know?

How I found it

In PR #794, I found the following variables had large diffs. I stepped through the code on main and the dev branch and found that main doesn't perform prs unit adjustment while the dev branch does.

Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_9.4_MISR-ANN-global_ref.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_9.4_MISR-ANN-global_ref.nc
    * var_key: CLDLOW_TAU1.3_9.4_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 39457 / 64800 (60.9%)
Max absolute difference: 22.411116
Max relative difference: 0.6832267
 x: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
 y: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_9.4_MISR-ANN-global_test.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_9.4_MISR-ANN-global_test.nc
    * var_key: CLDLOW_TAU1.3_9.4_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 9 / 64800 (0.0139%)
Max absolute difference: 0.0970192
Max relative difference: 0.01244658
 x: array([[ 0.056777,  0.056777,  0.056777, ...,  1.274017,  1.274017,
         1.274017],
       [ 0.207892,  0.207774,  0.207536, ...,  1.675944,  1.676576,...
 y: array([[ 0.056777,  0.056777,  0.056777, ...,  1.274017,  1.274017,
         1.274017],
       [ 0.207892,  0.207774,  0.207536, ...,  1.675944,  1.676576,...
Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_9.4_MISR-JJA-global_ref.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_9.4_MISR-JJA-global_ref.nc
    * var_key: CLDLOW_TAU1.3_9.4_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 34699 / 64800 (53.5%)
Max absolute difference: 45.429226
Max relative difference: 0.9708206
 x: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
 y: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...

------------------------------------------------------------------------------------------------------------------------------------------------

Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_MISR-ANN-global_ref.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_MISR-ANN-global_ref.nc
    * var_key: CLDLOW_TAU1.3_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 39499 / 64800 (61%)
Max absolute difference: 37.673122
Max relative difference: 0.62295455
 x: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
 y: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_MISR-ANN-global_test.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_MISR-ANN-global_test.nc
    * var_key: CLDLOW_TAU1.3_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 9 / 64800 (0.0139%)
Max absolute difference: 0.0970192
Max relative difference: 0.00541773
 x: array([[5.677656e-02, 5.677656e-02, 5.677656e-02, ..., 1.274017e+00,
        1.274017e+00, 1.274017e+00],
       [2.078919e-01, 2.077735e-01, 2.075364e-01, ..., 1.675944e+00,...
 y: array([[5.677656e-02, 5.677656e-02, 5.677656e-02, ..., 1.274017e+00,
        1.274017e+00, 1.274017e+00],
       [2.078919e-01, 2.077735e-01, 2.075364e-01, ..., 1.675944e+00,...
Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_MISR-JJA-global_ref.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU1.3_MISR-JJA-global_ref.nc
    * var_key: CLDLOW_TAU1.3_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 35149 / 64800 (54.2%)
Max absolute difference: 67.89603
Max relative difference: 0.9691263
 x: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
 y: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...

------------------------------------------------------------------------------------------------------------------------------------------------

Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU9.4_MISR-ANN-global_ref.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU9.4_MISR-ANN-global_ref.nc
    * var_key: CLDLOW_TAU9.4_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 39323 / 64800 (60.7%)
Max absolute difference: 31.085188
Max relative difference: 0.96666664
 x: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
 y: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...

Comparing:
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/792-lat-lon/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU9.4_MISR-JJA-global_ref.nc
    * /global/cfs/cdirs/e3sm/www/cdat-migration-fy24/main/lat_lon/Cloud MISR/MISRCOSP-CLDLOW_TAU9.4_MISR-JJA-global_ref.nc
    * var_key: CLDLOW_TAU9.4_MISR

Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 33486 / 64800 (51.7%)
Max absolute difference: 63.126827
Max relative difference: 1.
 x: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...
 y: array([[nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],
       [nan, nan, nan, ..., nan, nan, nan],...

Affects the following derived variables:

e3sm_diags/e3sm_diags/derivations/acme.py

Lines 1362 to 1409 in 43f5f74

"CLDLOW_TAU1.3_MISR": OrderedDict(
[
(
("CLD_MISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 1.3, None), target_units="%"
),
),
(
("CLMISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 1.3, None), target_units="%"
),
),
]
),
"CLDLOW_TAU1.3_9.4_MISR": OrderedDict(
[
(
("CLD_MISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 1.3, 9.4), target_units="%"
),
),
(
("CLMISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 1.3, 9.4), target_units="%"
),
),
]
),
"CLDLOW_TAU9.4_MISR": OrderedDict(
[
(
("CLD_MISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 9.4, None), target_units="%"
),
),
(
("CLMISR",),
lambda cld: convert_units(
cosp_bin_sum(cld, 0, 3, 9.4, None), target_units="%"
),
),
]
),

Environment

main and all versions of e3sm_diags that has cosp_bin_sum()
@tomvothecoder tomvothecoder added the bug Bug fix (will increment patch version) label Mar 6, 2024
@tomvothecoder tomvothecoder changed the title [Bug]: Silent bug in adjust_prs_val_units() conditional where if prs_val0: will be skipped if prs_val0 is 0 [Bug]: Silent bug in adjust_prs_val_units() conditional where if prs_val0: will be skipped if prs_val0 is 0 Mar 6, 2024
@tomvothecoder tomvothecoder mentioned this issue Mar 6, 2024
Merged
21 tasks
@tomvothecoder tomvothecoder changed the title [Bug]: Silent bug in adjust_prs_val_units() conditional where if prs_val0: will be skipped if prs_val0 is 0 [Bug]: Silent bug in adjust_prs_val_units() conditional where if prs_val0: will be False if prs_val0 is 0 Mar 6, 2024
@tomvothecoder
Copy link
Collaborator Author

tomvothecoder commented Mar 6, 2024 •
edited
Loading

These are the kinds of issues that can be easy to miss, which is why linters and type checkers (e.g., flake8 and mypy) are valuable and great to use as preventative tools.

@tomvothecoder tomvothecoder mentioned this issue Sep 27, 2024
Merged
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Bug fix (will increment patch version)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@tomvothecoder