Open tomvothecoder opened 9 months ago
Hi @chengzhuzhang, I believe I found a bug in the old version of cosp_histogram_standardize()
. Let me know if you agree with my solution, which includes adding missing bounds dynamically.
I might need help understanding the logic for the function that dynamically generates bounds (e.g., use coordinates as midpoints?).
Apologize for missing out this bug report! It is an interesting silence bug. And it almost suggests that the bounds are never used in the downstream calculation? I will have to look more.
Apologize for missing out this bug report! It is an interesting silence bug. And it almost suggests that the bounds are never used in the downstream calculation? I will have to look more.
As far as I can tell, the old CDAT version of cosp_histogram_standardize() and cosp_histogram_driver.py don't reference prs or tau bounds for any calculations. cosp_histogram_standardize()
subsets the variable using the low and high values of prs and/or tau, not prs bounds and/or tau bounds.
Hi Tom. I agree with your assessment that the cosp_histogram_standardize() are never actually used in the cdat version of the code. The conditions such as prs_bounds is None
is never meet, because prs_bounds always has a string type, if the bound attribute exist. And since the bounds are not referenced in calculation, we can perhaps remove this function?
Thanks for investigating @chengzhuzhang.
We can simplify the codebase by removing this function since it doesn't seem necessary to keep. Bounds are not used for calculations and you confirmed that newer model data and reference data have bounds (your comment).
What happened?
In PR #748, I found a silent bug in a conditional statement for handling bounds in
cosp_histogram_standardize()
.https://github.com/E3SM-Project/e3sm_diags/blob/633b52c314325e605fe7f62687cc4d00e5a0a3d5/e3sm_diags/derivations/acme.py#L552-L565
The conditional follows this flow:
cloud_prs_bounds
) might not align with the length of the coordinates, resulting in this error:ValueError: conflicting sizes for dimension 'misr_cth': length 7 on the data but length 16 on coordinate 'misr_cth'
What did you expect to happen? Are there are possible answers you came across?
In my version of the code, I get the "bounds" attr AND check if the bounds actually exist in the dataset.
If bounds don't exist in the dataset, bounds should be dynamically generated to align with the length of the coordinates instead of using a fixed bounds array (I think?).
Minimal Complete Verifiable Example (MVCE)
Relevant log output
No response
Anything else we need to know?
No response
Environment
Latest
main
version of e3sm_diags and all previous versions that use thecosp_histogram_standarize()
function.