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.

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

Remove the use of average_* variables and use time_bnds variables instead #278

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

uramirez8707
Copy link
Contributor

@uramirez8707 uramirez8707 commented Mar 25, 2024

Fixes #273
Fixes #274

I tried this using output from the old diag manager (which includes the average_* variables) and this updated reproduces the master branch.

I tried this using output from the new diag manager (which does not have the average_* variables). This update reproduces the output from the old diag manager.

@uramirez8707
Copy link
Contributor Author

FYI @ceblanton

@uramirez8707 uramirez8707 marked this pull request as ready for review March 27, 2024 20:20
@ilaflott ilaflott self-assigned this Mar 28, 2024
@ilaflott
Copy link
Contributor

tested on GFDL work-station, using a NetCDF file from fre-python-tool's time-averager.

exact function call to create averaged file: ./postprocessing/timavg/timavg.csh -r8 -mb -o FOO atmos.197901-198312.LWP.nc

compared against output of old timavg.csh output genereated for fre-python-tools tests.

stored the header info from ncdump -h into text files, then diff, yields

1c1
< netcdf FOO {
---
> netcdf fre_nctools_timavg_CLI_test_r8_mb_atmos_LWP_1979_5y {
16a17
> 		LWP:time_avg_info = "average_T1,average_T2,average_DT" ;
46a48,56
> 	double average_T1(time) ;
> 		average_T1:long_name = "Start time for average period" ;
> 		average_T1:units = "days since 1979-01-01 00:00:00" ;
> 	double average_T2(time) ;
> 		average_T2:long_name = "End time for average period" ;
> 		average_T2:units = "days since 1979-01-01 00:00:00" ;
> 	double average_DT(time) ;
> 		average_DT:long_name = "Length of average period" ;
> 		average_DT:units = "days" ;

this is the difference in metadata desired and described in issue 273 and issue 274. so, YAY!

numerical results bitwise identical as well, can be seen by ncdiff calls between the two files.

Copy link
Contributor

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all looks good to me

@ceblanton
Copy link
Contributor

Let's confirm that timavg.csh chokes as we think it does and should if the time_bounds are missing.

if timavg.csh hard fails when run on old/uncompliant history files (that don't have time_bounds) no one will end up with incorrect output, at least.

@uramirez8707
Copy link
Contributor Author

Let's confirm that timavg.csh chokes as we think it does and should if the time_bounds are missing.

if timavg.csh hard fails when run on old/uncompliant history files (that don't have time_bounds) no one will end up with incorrect output, at least.

This is what I get when I use a file that does not have the time_bounds:

 NETCDF ERROR in program time_average
 error getting varid for time bounds
 NetCDF: Variable not found

Program aborted. Backtrace:
#0  0x402c39 in ???
#1  0x403a63 in ???
#2  0x40b554 in ???
#3  0x40240c in ???
#4  0x2b70608e5554 in ???
#5  0x40243c in ???
#6  0xffffffffffffffff in ???
Abort

@ceblanton
Copy link
Contributor

Thanks for checking that. I'll merge and deploy into fre-nctools/test at GFDL now.

@ceblanton ceblanton self-requested a review March 29, 2024 17:30
@ceblanton ceblanton merged commit 79f5faa into NOAA-GFDL:master Mar 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fregrid should use standard time variables only timeaveraging tool should use standard time variables only
3 participants