When we switch to the netCDF3 interface all function calls are of the
form
rcd=nc_var_foo(nc_id,var,...);
where there are about a hundred different functions of this form
(one for each type of variable and each type of action).
When rcd != NC_NOERR then an error has occured and should
be handled (granted NCO error-checking is currently haphazard).
Instead of checking and handling errors in the main
code, we want to wrap each native netCDF call with a similarly
named NCO function that calls a single, universal error handler.
e.g., we use
rcd=nco_var_foo(nc_id,var,...) which calls nc_var_foo() internally
and, in addition has an error handling block tacked on like
if(rcd != NC_NOERR) nco_handle_netCDF_error(rcd);
and nco_handle_netCDF_error(rcd) does something like
{fprintf "%s", nc_strerror(rcd)); exit(-1); for now.
See the netCDF manual p. 30 for detailed example.
Then all the netCDF error handling goes through a single function,
nco_handle_netCDF_error() rather than cluttering up code
everywhere.
The work required to adapt the netCDF3 interface is substantial,
but probably worthwhile since it is 50% faster than netCDF2.
You should try converting one or a few functions first to get a
feeling for what is required. Then we can discuss your ideas
for an implementation in more detail. It's even possible that a
C++ solution would be more optimal than a C solution, but
I'd need to be convinced that is the case.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Charlie,
Have started on the netCDF3.x functions.
Please see patch_netcdf.
Have tried out the converted functions in cpy_var_val in ncks.c
Every thing seems to be OK
several points.
a) With netCDF3.x it seems impossible to detect the type text
( the nctypes are the same as netCDF2.x ).
I was getting an error message when reading "text" using
nc_get_vara_schar. So I am using the _text family of functions
in place of _schar family. This seems to work. This could cause some deep bugs to appear !!
b) I am typecasting the void pointers in the _get and _put function families to make the conversion process quick. Some compilers may complain about this.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I am examining the patch. Nice work so far.
It looks much like I expected.
I will send back a revision to the patch soon.
Let's not commit any of these netCDF 3 changes to CVS
until I release the backlog of new features that already exists,
i.e., at least nco 1.22 and 1.23. I'll try to do this in the next
few weeks. Then we can start the netCDF3 changes
as nco 1.3.x, and perhaps make a new CVS branch.
Here are my comments on the patch:
1. Since these are wrapper routines, let's preserve the netCDF
convention of verb_noun in routine names, e.g.,
nco_get_var1() instead of nco_var_get1().
This way wrapper name is nearly identical to routine name.
2. Make as many of the calling arguments to the wrappers
"const" as possible.
3. Initialize rcd to NC_NOERR and have wrapper return rcd
rather than return 1.
4. Use NCO standard names instead of netCDF names in the
wrappers. e.g., nc_id instead of ncid, var_typ instead of typ,
srt (stands for start, oddly enough) instead of indexp, var_val
instead of ip. The reason is so the NCO code has internally-consistent
names for everything. If anyone gets motivated to do a native
HDF NCO implementation, this discipline will pay off big time.
Because then, rather than having to remember the NCO, HDF,
and netCDF names for the same variable, we just need to use
the NCO name for everything. The idea is that NCO can evolve
to have its own internal representation of data, and, at a different
level, NCO communicates to files using the appropriate library,
e.g., netCDF or HDF. But internally we want NCO to be as
self-consistent as possible.
5. More comments in the headers and add the function name
at the bottom, if you can. Adding these comments make life easier
later on, and it's usually easy to cut and paste them from other
function.
About your other points,
Using the _text family is probably best, but _uchar should
also work. _schar should not work because text characters
are unsigned, i.e., 0--255, not signed -127--+128. Here is
my question to you: Why does nc.h define ptr_unn and val_unn
to use signed char for NC_CHAR buffers? I have no idea
why I defined them this way.
I do not know if it is right or wrong, but right now I'm thinking
that those should be changed to unsigned char. Perhaps
you could look into this and perform some tests as to which
behaves better.
About the typecasting. There are three alternatives:
1. Never typecast void pointers. This requires writing a gazillion
wrappers, one for each netCDF routine, and thus moving the
case statements into the main code. Both are unacceptable.
2. Cast as you are already doing. This is fine, and no case
statements need to be in the calling code. I am concerned about
compiler warning messages. It's important to me that the code compiles
cleanly with gcc -Wall or g++ -Wall with no warnings. Other compilers
I do not really care about. But if you cannot get gcc/g++ to compile
cleanly then stop what you are doing and we should rethink the
API.
3. Use C++ overloading and switch to C++. This is also fine, but
would take much longer as there are many more unknowns in moving
to C++, including the fact there is a netCDF C++ library wrapper
already and the fact that to cleanly take advantage of a C++ overloaded
API most of the existing NCO structures would have to be ported to
C++, i.e., all the void *'s would need to be removed.
The netCDF C++ wrapper from Unidata works but seems cumbersome
(although this may be unavoidable when doing generic programming like NCO).
Also, I think the netCDF C++ wrapper library is built on netCDF2
definitions so there may not be a performance boost even after
all that work (not sure about this, should ask Unidata).
So this option requires lots of planning and probably could not
be accomplished in an evolutionary way.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Charlie,
Been having a read of the netCDF C manual.According to this
NC_CHAR should definitely be unsigned.The manual is vague
about NC_BYTE I quote
"It is possible to interpret byte data as either signed (-128
to 127) or unsigned (0 to 255). However, when reading byte data to be converted into other numeric types, it is interpreted as signed."
However netCDF.h has the line
"NC_BYTE = 1, /* " signed 1 byte integer" */
Also ncgen treats byte variables as signed.
So I guess we should as well.
That makes two unenviable changes to make -- NC_CHAR to
unsigned and NC_BYTE to signed. You got them the wrong way round bro'.
The sensible thing would be to change them now rather than
propagate the mistakes in the wrapper functions. I 've had a quick look, making the changes is not that bad. Most of the
changes are in nc_utils.c. I don't think the changes will cause
severe compatability problems as the netCDF2 library probably
reads them correctly before converting to the "wrong" types.
How and when do you want me to tackle it ?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yikes! Well it's always better to find the big bugs before the users do!
Actually it's not surprising that this hasn't shown up as a problem.
People rarely use NC_BYTE. People use NC_CHAR all the time, but
the vast majority store strings in it, and English strings, at least, are
always represented by the 7 bit ASCII characters so values from 128--255
are very rare. Anyway, it would be great if you would take a crack at
fixing this.
Please sync to the current repository, implement the fix, and see whether
reading the NC_CHAR values in in.nc still works. Let me know if there
are any user-visible side effects. If your fix works go ahead and commit
it and we'll include it in the coming 1.2.2 release.
Thanks!
Charlie
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Charlie,
Have commited to above fix . the affected
files are ncatted.c , nc.h, nc_utl.c. Everything seems to work
ok --
I took nc_error exit out of nc_utl.c so I can use it for my wrapper
functions
regards henry
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Charlie,
Have completed the wrapper functions ( please see my latest
patch ) . How do you want to proceed with updating to netCDF3
the project ? Maybe I could just update ncks.c to start with ? It
would give us something to benchmark !
Regards Henry
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Nice work on the wrappers, the look clean and ready to go.
Yes, I'd say start with one operator as a proof of concept.
ncks is fine, but will probably take the most time to convert
to netcdf3. ncrename or ncatted are probably the easiest
operators to convert.
I've created a new NCO branch called netcdf3 using
cd ~/nco
cvs tag -b netcdf3
I want to you to begin to check in your changes on this branch.
To do this you will need to checkout NCO from the repository
using a special command,
cvs co -r netcdf3 -kk nco
or you can switch your existing working code to be on that branch
using
cd ~/nco
cvs update -r netcdf3
As you proceed with the netcdf3 implementation, check in your
changes. This will be the nco 1.3.x series. I've already tagged
the existing base code of the netcdf3 branch with the version
nco-1_3. Your first changes (the wrapper functions) should be
committed to the repository (e.g., in nco_wrp.c), and, once they
build, given version tag nco-1_3_1. Increment the 1.3.x minor version
numbers as you see fit (nco-1_3_2, ....).
I know all this CVS stuff is confusing at first, but give it a try
and read some of the CVS manual. When I finish the remaining
nco 1.2 releases in a few weeks then I will switch the code so that
the netcdf3 branch becomes the main trunk. By that time hopefully
you will have some entire operators converted, and we can begin
releasing them publicly.
Thanks,
Charlie
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Charlie,
I'm getting warnings from the compiler (gcc) about incompatible
pointer types when calling my wrapper functions. These
occur in the functions where the arguments are of type
" const size_t " or size_t ( many of the arguments in the
orignal netCDF3 functions are of type size_t ). what do
suggest I do about them ? Maybe I could recast the
the pointers to type size_t in the wrapper functions ?
The fastest way for me to update to netCDF3 is to do it all in
one go rather than file by file. I use the package sniff for project
management and it has a very good global search and replace
utility.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yes, I noticed the pointers had changed from long * to size_t *.
The smart thing to do is to change NCO to size_t where necessary.
We want NCO to fully switch to netCDF 3 compatibility, so we
must follow the netCDF library routine definitions.
Where this involves tinkering with the difference between
long and nclong it may be tricky. I don't know but this is an example
of why changing and testing one operator (e.g., ncks) first
might be wise.
64 bit machines like SGIs and Compaq Alphas are where I
expect the problems, if any, will show up. In case you don't
have access to such machines, I can run nco_tst.sh on them
once you have an NCO 1.3.x version that you expect will run.
I'm not sure I answered your original question. If not, let me know
and send me an example of any problems you're running into.
Thanks,
Charlie
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I see the name-space issue as separate from the file splitting issue.
Routines (and structures) which require netCDF capability or are NCO-specific should have an nco_ prefix. These happen to all
be in nc_utl.c currently, but nc_utl.c should be thoughtfully
dismembered--it's too big. Routines (and structures) which are
more generic, not netCDF/NCO-specific should be given a
different prefix so they may be put into a separate library which
can be used by other software which has no knowledge about NCO.
Currently these routines are in csz.c and csz_ is a good prefix for now.
pck.c, which contains the new packing routines, is a good example
of a more rational file structure than nc_utl.c. pck.c contains all the
routines specific to packing. Ideally it would have its own header,
pck.h, which would be split off from nc.h, which is now a global header.
nc.h should eventually become nco.h and just #include all the headers
of the other nco library files, e.g. pck.h, var.h, dmn.h, avg.h...
Similarly there should be a csz.h for the NCO-indenpent routines.
That's the long term goal, anyway. For now we just want to start
moving routines which are rationally related into smaller, more
self contained files. It can be done in stages, there's no reason
to try to do it all at once, except, possibly, over-caffeination :)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Charlie,
Need further clarification on this one, what do the wrappers
what do the wrappers that call nc_error go round ?
Also it seems a bit daft to embark on TODO 82 , when nc_utils
needs breaking up into smaller files ??
regards Henry
When we switch to the netCDF3 interface all function calls are of the
form
rcd=nc_var_foo(nc_id,var,...);
where there are about a hundred different functions of this form
(one for each type of variable and each type of action).
When rcd != NC_NOERR then an error has occured and should
be handled (granted NCO error-checking is currently haphazard).
Instead of checking and handling errors in the main
code, we want to wrap each native netCDF call with a similarly
named NCO function that calls a single, universal error handler.
e.g., we use
rcd=nco_var_foo(nc_id,var,...) which calls nc_var_foo() internally
and, in addition has an error handling block tacked on like
if(rcd != NC_NOERR) nco_handle_netCDF_error(rcd);
and nco_handle_netCDF_error(rcd) does something like
{fprintf "%s", nc_strerror(rcd)); exit(-1); for now.
See the netCDF manual p. 30 for detailed example.
Then all the netCDF error handling goes through a single function,
nco_handle_netCDF_error() rather than cluttering up code
everywhere.
The work required to adapt the netCDF3 interface is substantial,
but probably worthwhile since it is 50% faster than netCDF2.
You should try converting one or a few functions first to get a
feeling for what is required. Then we can discuss your ideas
for an implementation in more detail. It's even possible that a
C++ solution would be more optimal than a C solution, but
I'd need to be convinced that is the case.
Hi Charlie,
Have started on the netCDF3.x functions.
Please see patch_netcdf.
Have tried out the converted functions in cpy_var_val in ncks.c
Every thing seems to be OK
several points.
a) With netCDF3.x it seems impossible to detect the type text
( the nctypes are the same as netCDF2.x ).
I was getting an error message when reading "text" using
nc_get_vara_schar. So I am using the _text family of functions
in place of _schar family. This seems to work. This could cause some deep bugs to appear !!
b) I am typecasting the void pointers in the _get and _put function families to make the conversion process quick. Some compilers may complain about this.
Hi Henry,
I am examining the patch. Nice work so far.
It looks much like I expected.
I will send back a revision to the patch soon.
Let's not commit any of these netCDF 3 changes to CVS
until I release the backlog of new features that already exists,
i.e., at least nco 1.22 and 1.23. I'll try to do this in the next
few weeks. Then we can start the netCDF3 changes
as nco 1.3.x, and perhaps make a new CVS branch.
Here are my comments on the patch:
1. Since these are wrapper routines, let's preserve the netCDF
convention of verb_noun in routine names, e.g.,
nco_get_var1() instead of nco_var_get1().
This way wrapper name is nearly identical to routine name.
2. Make as many of the calling arguments to the wrappers
"const" as possible.
3. Initialize rcd to NC_NOERR and have wrapper return rcd
rather than return 1.
4. Use NCO standard names instead of netCDF names in the
wrappers. e.g., nc_id instead of ncid, var_typ instead of typ,
srt (stands for start, oddly enough) instead of indexp, var_val
instead of ip. The reason is so the NCO code has internally-consistent
names for everything. If anyone gets motivated to do a native
HDF NCO implementation, this discipline will pay off big time.
Because then, rather than having to remember the NCO, HDF,
and netCDF names for the same variable, we just need to use
the NCO name for everything. The idea is that NCO can evolve
to have its own internal representation of data, and, at a different
level, NCO communicates to files using the appropriate library,
e.g., netCDF or HDF. But internally we want NCO to be as
self-consistent as possible.
5. More comments in the headers and add the function name
at the bottom, if you can. Adding these comments make life easier
later on, and it's usually easy to cut and paste them from other
function.
About your other points,
Using the _text family is probably best, but _uchar should
also work. _schar should not work because text characters
are unsigned, i.e., 0--255, not signed -127--+128. Here is
my question to you: Why does nc.h define ptr_unn and val_unn
to use signed char for NC_CHAR buffers? I have no idea
why I defined them this way.
I do not know if it is right or wrong, but right now I'm thinking
that those should be changed to unsigned char. Perhaps
you could look into this and perform some tests as to which
behaves better.
About the typecasting. There are three alternatives:
1. Never typecast void pointers. This requires writing a gazillion
wrappers, one for each netCDF routine, and thus moving the
case statements into the main code. Both are unacceptable.
2. Cast as you are already doing. This is fine, and no case
statements need to be in the calling code. I am concerned about
compiler warning messages. It's important to me that the code compiles
cleanly with gcc -Wall or g++ -Wall with no warnings. Other compilers
I do not really care about. But if you cannot get gcc/g++ to compile
cleanly then stop what you are doing and we should rethink the
API.
3. Use C++ overloading and switch to C++. This is also fine, but
would take much longer as there are many more unknowns in moving
to C++, including the fact there is a netCDF C++ library wrapper
already and the fact that to cleanly take advantage of a C++ overloaded
API most of the existing NCO structures would have to be ported to
C++, i.e., all the void *'s would need to be removed.
The netCDF C++ wrapper from Unidata works but seems cumbersome
(although this may be unavoidable when doing generic programming like NCO).
Also, I think the netCDF C++ wrapper library is built on netCDF2
definitions so there may not be a performance boost even after
all that work (not sure about this, should ask Unidata).
So this option requires lots of planning and probably could not
be accomplished in an evolutionary way.
Hi Charlie,
Been having a read of the netCDF C manual.According to this
NC_CHAR should definitely be unsigned.The manual is vague
about NC_BYTE I quote
"It is possible to interpret byte data as either signed (-128
to 127) or unsigned (0 to 255). However, when reading byte data to be converted into other numeric types, it is interpreted as signed."
However netCDF.h has the line
"NC_BYTE = 1, /* " signed 1 byte integer" */
Also ncgen treats byte variables as signed.
So I guess we should as well.
That makes two unenviable changes to make -- NC_CHAR to
unsigned and NC_BYTE to signed. You got them the wrong way round bro'.
The sensible thing would be to change them now rather than
propagate the mistakes in the wrapper functions. I 've had a quick look, making the changes is not that bad. Most of the
changes are in nc_utils.c. I don't think the changes will cause
severe compatability problems as the netCDF2 library probably
reads them correctly before converting to the "wrong" types.
How and when do you want me to tackle it ?
Yikes! Well it's always better to find the big bugs before the users do!
Actually it's not surprising that this hasn't shown up as a problem.
People rarely use NC_BYTE. People use NC_CHAR all the time, but
the vast majority store strings in it, and English strings, at least, are
always represented by the 7 bit ASCII characters so values from 128--255
are very rare. Anyway, it would be great if you would take a crack at
fixing this.
Please sync to the current repository, implement the fix, and see whether
reading the NC_CHAR values in in.nc still works. Let me know if there
are any user-visible side effects. If your fix works go ahead and commit
it and we'll include it in the coming 1.2.2 release.
Thanks!
Charlie
Hi Charlie,
Have commited to above fix . the affected
files are ncatted.c , nc.h, nc_utl.c. Everything seems to work
ok --
I took nc_error exit out of nc_utl.c so I can use it for my wrapper
functions
regards henry
Hi Charlie,
Have completed the wrapper functions ( please see my latest
patch ) . How do you want to proceed with updating to netCDF3
the project ? Maybe I could just update ncks.c to start with ? It
would give us something to benchmark !
Regards Henry
Henry,
Nice work on the wrappers, the look clean and ready to go.
Yes, I'd say start with one operator as a proof of concept.
ncks is fine, but will probably take the most time to convert
to netcdf3. ncrename or ncatted are probably the easiest
operators to convert.
I've created a new NCO branch called netcdf3 using
cd ~/nco
cvs tag -b netcdf3
I want to you to begin to check in your changes on this branch.
To do this you will need to checkout NCO from the repository
using a special command,
cvs co -r netcdf3 -kk nco
or you can switch your existing working code to be on that branch
using
cd ~/nco
cvs update -r netcdf3
As you proceed with the netcdf3 implementation, check in your
changes. This will be the nco 1.3.x series. I've already tagged
the existing base code of the netcdf3 branch with the version
nco-1_3. Your first changes (the wrapper functions) should be
committed to the repository (e.g., in nco_wrp.c), and, once they
build, given version tag nco-1_3_1. Increment the 1.3.x minor version
numbers as you see fit (nco-1_3_2, ....).
I know all this CVS stuff is confusing at first, but give it a try
and read some of the CVS manual. When I finish the remaining
nco 1.2 releases in a few weeks then I will switch the code so that
the netcdf3 branch becomes the main trunk. By that time hopefully
you will have some entire operators converted, and we can begin
releasing them publicly.
Thanks,
Charlie
Hi Charlie,
I'm getting warnings from the compiler (gcc) about incompatible
pointer types when calling my wrapper functions. These
occur in the functions where the arguments are of type
" const size_t " or size_t ( many of the arguments in the
orignal netCDF3 functions are of type size_t ). what do
suggest I do about them ? Maybe I could recast the
the pointers to type size_t in the wrapper functions ?
The fastest way for me to update to netCDF3 is to do it all in
one go rather than file by file. I use the package sniff for project
management and it has a very good global search and replace
utility.
Hi Charlie,
Just to further clarify the above problem.
The long * pointers in netCDF2.x have been replaced by
size_t * pointers in netCDF3.x
regards henry
PS I hope Gore wins !!
Yes, I noticed the pointers had changed from long * to size_t *.
The smart thing to do is to change NCO to size_t where necessary.
We want NCO to fully switch to netCDF 3 compatibility, so we
must follow the netCDF library routine definitions.
Where this involves tinkering with the difference between
long and nclong it may be tricky. I don't know but this is an example
of why changing and testing one operator (e.g., ncks) first
might be wise.
64 bit machines like SGIs and Compaq Alphas are where I
expect the problems, if any, will show up. In case you don't
have access to such machines, I can run nco_tst.sh on them
once you have an NCO 1.3.x version that you expect will run.
I'm not sure I answered your original question. If not, let me know
and send me an example of any problems you're running into.
Thanks,
Charlie
I see the name-space issue as separate from the file splitting issue.
Routines (and structures) which require netCDF capability or are NCO-specific should have an nco_ prefix. These happen to all
be in nc_utl.c currently, but nc_utl.c should be thoughtfully
dismembered--it's too big. Routines (and structures) which are
more generic, not netCDF/NCO-specific should be given a
different prefix so they may be put into a separate library which
can be used by other software which has no knowledge about NCO.
Currently these routines are in csz.c and csz_ is a good prefix for now.
pck.c, which contains the new packing routines, is a good example
of a more rational file structure than nc_utl.c. pck.c contains all the
routines specific to packing. Ideally it would have its own header,
pck.h, which would be split off from nc.h, which is now a global header.
nc.h should eventually become nco.h and just #include all the headers
of the other nco library files, e.g. pck.h, var.h, dmn.h, avg.h...
Similarly there should be a csz.h for the NCO-indenpent routines.
That's the long term goal, anyway. For now we just want to start
moving routines which are rationally related into smaller, more
self contained files. It can be done in stages, there's no reason
to try to do it all at once, except, possibly, over-caffeination :)