NCAR / ccpp-scm

CCPP Single Column Model
Other
13 stars 50 forks source link

Fix output-related bugs #293

Closed grantfirl closed 2 years ago

grantfirl commented 2 years ago

This should cause failed SCM RTs due to changed output files.

grantfirl commented 2 years ago

Replaces #292

mkavulich commented 2 years ago

Looks good aside from the issue Dom brought up and my one comment.

Can you document what tests were run for this PR?

grantfirl commented 2 years ago

Thanks @climbfuji and @mkavulich for the review and suggestions, particularly pointing out the optional argument problem that I introduced. For testing, I tried several different combinations of n_itt_out and n_itt_diag in the case configuration files. There had been a bug where the instantaneous variables were trying to be written out one too many times with some combinations of these values. This is definitely fixed. I ran the SCM RTs and the results were as-expected: all tests pass with some differences to baselines. I checked the differences using nccmp manually and the only difference was a change in data type from float->integer for the expected variables. This is the expected result. I also plotted some results from the TWPICE case using all supported suites and everything looked good except for some of the tendency plots were plotting the FILL values as if they were good values. Some changes to the scm_analysis.py script fixed that and the plots are now good to go for now.

MicroTed commented 2 years ago

except for some of the tendency plots were plotting the FILL values as if they were good values

I noticed this issue in recent tests -- thanks for fixing it!

grantfirl commented 2 years ago

fixes #294

MicroTed commented 2 years ago

@grantfirl By the way, the NaN conversion is clever, but gives me this error (anconda, Python 3.8.5 and 3.8.12):

raise ValueError("Axis limits cannot be NaN or Inf") ValueError: Axis limits cannot be NaN or Inf

It works if I set the values to zero instead of np.nan. I've seen that NaN values are supposed to be skipped in the plotting, but maybe it doesn't like having all the values be nan?