search

CH-Earth / summa

Structure for Unifying Multiple Modeling Alternatives:
http://www.ral.ucar.edu/projects/summa
GNU General Public License v3.0
79 stars 103 forks source link

hotfix/removed deflate_level setting from restart file generation #494

Closed wknoben closed 2 years ago

wknoben commented 2 years ago

New deflate_level setting causes errors when writing restart files that lead to graceful (but unreadable) model failure:

1979 12 31  0  0

WARNING: f-writeTime/writeRestart/Ðh/@ý                                             àþÿÿÿÿÿÿÿÿ  ÿÿÿÿÿÿ              ÿÿ€  ±4”B“=  8"  ðŽG    ÿÿÿÿÿÿÿÿ                          $i/@ý                                      0i/@ý     f+  x                 
 (can keep going, but stopping anyway)

Some digging suggests that these are incorrectly handled errors that should read:

writeRestart/[NetCDF: Attempting netcdf-4 operation on netcdf-3 file][NetCDF: Attempting netcdf-4 operation on netcdf-3 file]

Restart files are indeed created with netCDF-3 data models https://github.com/CH-Earth/summa/blob/372c3fbeb3825e3b3d635461a8e552f9f0895aec/build/source/netcdf/modelwrite.f90#L546 whereas regular output files are indeed created with netCDF-4 support https://github.com/CH-Earth/summa/blob/372c3fbeb3825e3b3d635461a8e552f9f0895aec/build/source/netcdf/def_output.f90#L217

indicating that setting the deflate_level on netCDF-3 files is not supported. Because this code is are currently live on develop and effectively prevents the use of restart files, I decided to simply remove the deflate_level option from restart files for the moment until we can have a deeper look at this (possibly change restart files to netCDF-4?). Brief tests with updated code successfully generate a restart file.

andywood commented 2 years ago

I think that having a single fixed format on the restart files (write and read) is a good choice (why not netcdf4?). It avoids any potential inconsistency in state files being written and then unusable for starting a different run.

wknoben commented 2 years ago

I didn't know if there were any (legacy?) reasons to have the restarts as netcdf3 so it seemed safer not to touch that in this fix and just remove the new code that was causing an issue. If we can test having restarts as netcdf4 through a regular reviewed PR then I'm all for it.