Use correct NetCDF types when writing attributes #363
Conversation
This way we can use them from elsewhere. Also, add some error checking of their return value.
|
@uturuncoglu - I have tested that this fixes the issue in the little test program that you helped me create, so I expect this will resolve your issue, but it would be great if you're able to test with this branch and verify that. @oehmke I'd welcome a quick review of this to verify that my creation of these standalone utility functions follows ESMF best practices, e.g., with the use of namespaces. (I found a few examples in the ESMF code, but it didn't feel like things were being done consistently, so I made my best attempt at what felt like good practices.) I still want to:
|
| namespace ESMCI{ | ||
| namespace NetCDFUtils{ |
There was a problem hiding this comment.
Note that I put these functions in a namespace nested inside the ESMCI namespace. Is this a reasonable approach?
| if (arrayType == ESMF_NOKIND) { | ||
| string errstr = string(": problem converting NetCDF type to ESMF type"); | ||
| ESMC_LogDefault.Write(errstr, ESMC_LOGMSG_ERROR, ESMC_CONTEXT); | ||
| *rc = ESMF_FAILURE; | ||
| return thisArray; | ||
| } |
There was a problem hiding this comment.
Note the error handling that I've added here. The original function didn't have an rc return value, and I kept that interface – but I could change this to have an explict rc if you feel that would be better. (I'm not sure what the accepted approach is for low-level internal utility functions like this.)
| if (ncType == NC_UNSPECIFIED) { | ||
| string errstr = string(": problem converting ESMF type to NetCDF type"); | ||
| ESMC_LogDefault.MsgFoundError(ESMF_FAILURE, errstr, ESMC_CONTEXT, &localrc); | ||
| return localrc; | ||
| } |
There was a problem hiding this comment.
As above: Note the error handling that I've added here. The original function didn't have an rc return value, and I kept that interface – but I could change this to have an explict rc if you feel that would be better. (I'm not sure what the accepted approach is for low-level internal utility functions like this. Also, for this function, note that, in the new usage in the PIO handler, I deliberately don't check the return value because there may be cases where there's an unhandled type but that's okay.)
| string errstr = string(": unhandled NetCDF type"); | ||
| ESMC_LogDefault.MsgFoundError(ESMF_FAILURE, errstr, ESMC_CONTEXT, &localrc); | ||
| return localrc; |
There was a problem hiding this comment.
Note some more error handling here. I think it's correct to add this error handling instead of the previous behavior of quietly doing nothing in this case: I can't think of a reason why you'd want it to quietly do nothing here.
|
|
||
| ESMC_TypeKind_Flag esmcTypeVal = ESMF_NOKIND; | ||
|
|
||
| #if defined(ESMF_NETCDF) || defined(ESMF_PNETCDF) |
There was a problem hiding this comment.
I considered a few ways to do the conditional compilation here and in esmcToNcType. Based on a quick code analysis, I think we need these functions to be compiled even if we're not using netcdf or pnetcdf, but the return value is unimportant in those cases. I don't love putting essentially the whole body of the function inside an #if, but this seemed like the best way to do it - though I'm open to other suggestions.
(As a side note: I'd love if we could just say that netcdf is required, or have some intermediate layer between netcdf and esmf that we use from esmf to avoid a direct dependency, so we could avoid having #ifs like this in the code, because I find they make it much harder to read, write and modify code.)
This resolves the issue reported by @uturuncoglu in https://github.com/esmf-org/esmf-support/issues/507 - that NetCDF attributes were being written with the wrong types.
To do this, I extracted some existing NetCDF utility functions (which were private to the class IO_NetCDF) into a new utility module.
I have also added some error checking of the return value of these functions in a couple of places where, previously, these errors were unhandled.