Bathroom sink needs a few washers

[Charlie Zender]

Ho ho ho Henry,

Thanks for the commits. I can see the holidays are just
getting started now. I am moving this discussion to the
NCO developer list so other can follow your progress.

> Have commited the first version of ncks which can do
multi-hyperslabbing.
> It's still a bit rough round the edges but it works

Cool, Santa. nco_var_utl was a bit long already so I
created nco_msa.[ch] for the multi-slabbing algorithm
code. So update and continue development there.
One style point: please never use single letter variable
names, e.g., use idx instead of i, lmt_idx instead of lmt_i.
This will ease code maintenance.

There are some bugs you will need to fix before we can tag
this. I have not investigated these in detail so I do not
know how serious they are, but the GCC compiler points
them out when I compile. First two are highest priority
since they might lead to incorrect answers on some architectures.

1. Calling argument inconsisent with prototype in
    (void)nco_msa_calc_cnt(&lmt_lst[idx]);

2. Something appears inconsistent with call and prototype of
    (void)nco_cpy_var_val_multi_lmt(in_id,out_id,fp_bnr,NCO_BNR_WRT,xtr_lst[idx].nm,lmt_lst,nbr_dmn_fl);

3. Plenty of compiler warnings in nco_msa.c.
   Please fix as many as possible and then I'll give it a whack.
   Messages appended below.

4. The variable-size array being non-C89 warning is serious
   because non-GNU compilers will not, in general, support this so
   ncks will not build on many different platforms until these are
   addressed. Note that the other NCO codes that causes this warning
   is protected by #ifdef __GNUC__ (see nco_var_avg.c) so there is
   an alternative provided for non-__GNUC__ compilers. In other words
   we need a C89 code path that works with brain-dead compilers.

> features : - Can handle multiple wrapped dimensions
  Multiple hyperslabbing of any of the dimensions

Very nice. I have not tried it yet. I assume it continues
to pass the nco_tst.sh test. Can you add in some tests that
will fail if multi-slabbing is broken?

> problems:
  Bit of a memory hog - especially with multi-hyperslabbing

Multi-slabbing appears to be invoked even when no dimension
is hyperslabbed more than once. In other words, nco_cpy_var_val_lmt()
is now obsolete. Is memory use worse only when multi-slabbing is
actually used? If so, then obsoleting nco_cpy_var_val_lmt() is
fine since nco_cpy_var_val_multi_lmt() is more generic and has
no performance penalty. If not, then we should create interface
routine to decide between invoking nco_cpy_var_val_multi_lmt()
and onco_cpy_var_val_lmt(). Do you agree?

> nco_msa_calc_indices could be improved - -Maybe we could cashe the hyperslab
  indices ?

Caching indices

> Need to work out how to print the hyperslabs ?

This will be a real bear.

Also need to document new multi-slabbing capability in nco.texi
and add examples. This is really cool. I hope the LLNL authors
are not disappointed. They still have not shipped me the actual
code and GPL releases for ncbs.c

But all we need to do to tag, release, and announce is address #'s
1--4 above. Let me know when would be a good time for me to look at
nco_msa.c again.

> Any way        -- Have merry Christmas

Will do. You too.

Thanks!
Charlie

gcc -std=c99 -ansi -pedantic -D_BSD_SOURCE  -DLINUX -DNO_NETCDF_2 -DVERSION='20021219' -DHOSTNAME='dust.ps.uci.edu' -DUSER='zender'  -DHAVE_GETOPT_H -DHAVE_GETOPT_LONG -DHAVE_MKSTEMP -DHAVE_STRCASECMP -DHAVE_STRDUP -Di386 -I./ -I../src/nco -I/usr/local/include -U_OPENMP -Wall -O -c ../src/nco/nco_msa.c -o /home/zender/obj/LINUX/nco_msa.o
cc1: warning: changing search order for system directory "/usr/local/include"
cc1: warning:   as it has already been specified as a non-system directory
../src/nco/nco_msa.c: In function `nco_msa_merge_slabs':
../src/nco/nco_msa.c:25: warning: ISO C89 forbids variable-size array `indices'
../src/nco/nco_msa.c:68: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:70: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:73: warning: pointer of type `void *' used in subtraction
../src/nco/nco_msa.c:77: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:78: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:23: warning: unused variable `jdx'
../src/nco/nco_msa.c: In function `nco_msa_rec_calc':
../src/nco/nco_msa.c:127: warning: ISO C89 forbids variable-size array `vp_size'
../src/nco/nco_msa.c:128: warning: ISO C89 forbids variable-size array `indices'
../src/nco/nco_msa.c:130: warning: ISO C89 forbids variable-size array `vp_wrap'
../src/nco/nco_msa.c:173: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:174: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:179: warning: pointer of type `void *' used in subtraction
../src/nco/nco_msa.c:181: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:182: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:297: warning: pointer of type `void *' used in subtraction
../src/nco/nco_msa.c:300: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:302: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:303: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:304: warning: pointer of type `void *' used in arithmetic
../src/nco/nco_msa.c:320: warning: ISO C89 forbids variable-size array `dmn_srt'
../src/nco/nco_msa.c:321: warning: ISO C89 forbids variable-size array `dmn_cnt'
../src/nco/nco_msa.c:322: warning: ISO C89 forbids variable-size array `dmn_srd'
../src/nco/nco_msa.c:207: warning: `index' might be used uninitialized in this function
../src/nco/nco_msa.c: In function `nco_msa_print_indices':
../src/nco/nco_msa.c:360: warning: ISO C89 forbids variable-size array `indices'
../src/nco/nco_msa.c:363: warning: int format, long int arg (arg 3)
../src/nco/nco_msa.c:370: warning: int format, long int arg (arg 3)
../src/nco/nco_msa.c:370: warning: int format, long int arg (arg 4)
../src/nco/nco_msa.c:370: warning: int format, long int arg (arg 5)
../src/nco/nco_msa.c:370: warning: int format, long int arg (arg 6)
../src/nco/nco_msa.c:358: warning: unused variable `jdx'
../src/nco/nco_msa.c: In function `nco_msa_calc_indices':
../src/nco/nco_msa.c:390: warning: ISO C89 forbids variable-size array `min'
../src/nco/nco_msa.c:395: warning: `previous_index' might be used uninitialized in this function
../src/nco/nco_msa.c: In function `nco_msa_calc_cnt':
../src/nco/nco_msa.c:474: warning: ISO C89 forbids variable-size array `indices'
../src/nco/nco_msa.c:475: warning: ISO C89 forbids variable-size array `min'
../src/nco/nco_msa.c: In function `nco_cpy_var_val_multi_lmt':
../src/nco/nco_msa.c:620: warning: assignment discards qualifiers from pointer target type
../src/nco/nco_msa.c:539: warning: unused variable `SRD'
../src/nco/nco_msa.c:540: warning: unused variable `WRP'
../src/nco/nco_msa.c:545: warning: unused variable `dmn_idx'
../src/nco/nco_msa.c:546: warning: unused variable `lmt_idx'
../src/nco/nco_msa.c:554: warning: unused variable `dmn_cnt'
../src/nco/nco_msa.c:555: warning: unused variable `dmn_in_srt'
../src/nco/nco_msa.c:559: warning: unused variable `dmn_srd'
../src/nco/nco_msa.c:560: warning: unused variable `dmn_sz'

[henry Butowsky]

Hi Charlie, Sorry about the compiler warnings - In future I shall run gcc with -ansi flag.

Memory use with nco_cpy_var_val_multi_lmt() .
For single slabbed variables there should be hardly any difference to nco_cpy_var_val_lmt(). The new routine will simple recurse along a single branch.

Shall try to do the fixes this weekend or else after Xmas

Regards Henry

[Charlie Zender]

Hi Henry,

Good. It sounds like it's best to deprecate nco_cpy_var_val_lmt() for now.
After we're happy with the ncks implementation, maybe we can look at what it would take to
implement multislabbing in the arithmetic operators.

Also note that NCO function prototypes now use
"const" wherever possible. We are promulgating the
principle of least priveledges. Please "constify" your
function arguments where possible (_anything_ that
should not change should be const). There are some
portability macros in nco.h that handle tricky things like
const foo * const * const
that are legal in C++ but not in C99.
#ifdef __cplusplus
#define CST_X_PTR_CST_PTR_CST_Y(x,y) const x * const * const y
#define X_CST_PTR_CST_PTR_Y(x,y) x const * const * y
#else /* !__cplusplus */
#define CST_X_PTR_CST_PTR_CST_Y(x,y) x * const * const y
#define X_CST_PTR_CST_PTR_Y(x,y) x * const * y
#endif /* !__cplusplus */

Thanks,
Charlie

[Nobody/Anonymous]

Hi Charlie,
Have completed a general tidy up of nco_msa.   I tried compiling with the -ansi - pedantic flags but the standard headers wouldn't compile , let alone the project  !! - So you'll have to be my eyes and ears on this one.  Not sure what to do about the void ponter arithmetic  errors. I suppose I could change the type  to unsigned char ( I assume sizeof(uchar) is always 1 byte -regardless of the platform ???)  .

Going to Brussels tomorrow will be back home on friday -- Merry Xmas

regards Henry

[Charlie Zender]

Hi Henry,

Ooops, I see you already realized that changing to char *
is a potential solution. In practice, yes, sizeof(char)=1 on
all known platforms. But I do not know if this is technically
required by the C standard. If you do this, use char * rather
than unsigned char *, just to be consistent with nco_var_avg().

Thanks,
Charlie

[Charlie Zender]

Hi Henry,

FYI, the correct flags to compile NCO on Linux are

gcc -std=c99 -ansi -pedantic -D_BSD_SOURCE

Withou the BSD_SOURCE  you will get errors with the
system headers. if you use -ansi.

These are the default flags in the bld/akefile

[Charlie Zender]

Hi Henry,

I worked on cleaning up nco_msa.[ch], so please update.
Where possible I switched names to NCO standard names.
Please verify that the two initializations to long_CEWI to remove
compiler warnings are, in fact, OK, and actual running code will
never see these values.

I'm not positive what to do to remove the warnings about doing pointer
arithmetic with type void *, but I am 90% sure. I think GCC prints
warnings for void * arithmetic because before manipulating pointers,
one should always know what the size is of the elements they point to.
Thus GCC developers think that the information required to use
non-void pointers is in all cases present and should be used to avoid
possible mistakes. In any case, that is my viewpoint.

The situation in your code seems to be exactly analogous to what
occurs in nco_var_avg() where the val.vp is converted to a char *
before arithmetic is done with it. If you use this method then you
simply have to remember to  multiply your offsets by typ_sz before any
memcpy's. So please look at nco_var_avg() and see if you think
creating some _cp variables in nco_msa_rec_clc() would get rid of the
warnings with minimal overhead. This modification is deep in your
algorithm so I do not want to touch the code myself for fear of
breaking it. But we do need to do something to remove the GCC warning
messages. Let me know how you want to handle it.

Thanks,
Charlie

[henry Butowsky]

Hi Charlie,
Have changed the void -pointer arithmetic to char in nco_msa_rec_clc .

We need to work out how to do the printing -Also I thing nco_msa_rec_clc needs profiling,. If the last dim of a variable is large (i.e >1000) and has limits, then the indices are repeatedly calculaled for each hyperslab

regards Henry

[Charlie Zender]

>Have changed the void -pointer arithmetic to char in >nco_msa_rec_clc .

Thanks, the compiler warnings look much better now.
I still get warned that nco_msa_rec_clc() is implicitly defined,
though, which makes me think that the prototype does not match
the way the function is called. Does this happen to you?

I've added a section to the user's guide on multislabbing.
Please correct any  mistakes in it and expand as you see fit.

>We need to work out how to do the printing

For now can you just add this to the TODO list and print a warning when -H is used with multislabbing?

>Also I thing nco_msa_rec_clc needs profiling,. If the last dim >of a variable is large (i.e >1000) and has limits, then the >indices are repeatedly calculaled for each hyperslab

I'm not sure how serious this is, so yes, we need some
profiling. ncks is used in production environments by many scientists on files with dimensions larger than 1000.
Are you saying you expect things to be
significantly slower? Users will be upset if their hyperslabbing
slows to a crawl.  If so we need a workaround before
we announce the multislabbing capability.

I added some multislab tests to nco_tst.sh but have not
tried anything on variables with large last dimensions.
Please post your description of your algorithm to the thread
I began on this forum. I can't find that email from you...

Happy new year!
Charlie

[henry Butowsky]

Hello Charlie and Rorick -- Happy New Year
I have no problems with the nco_msa_rec_clc() function prototype.

Printing - Forgive me for stating the obvious but why can't we simply print out the resultant variable from nco_msa_rec_clc() or read the var from the output file ?

Caching indices :-      
The factors which I think determine the peformance of nco_msa_rec_clc() when multi-slabbing are :-
(1) The output dimension size
(2) The total number of slabs.
(3) The original dimension size.

My idea is to cache say 1000 slab indices :-
In certain cases it will not make a difference e.g when we have interlaced slabs -d time,,,3 -d time,,,4
and the input dimension size is very large

regards Henry

[Charlie Zender]

> I have no problems with the nco_msa_rec_clc() function prototype.

This is strange because I'm still getting link warnings with this.
But if no one else gets these warnings then it's probably just my
quirky old system begging to be put of its misery again.

> Printing - Forgive me for stating the obvious but why can't we
> simply print out the resultant variable from nco_msa_rec_clc() or
> read the var from the output file ? 

ncks currently separates printing from hyperslabbing routines for
mainly historical reasons. ncks is invokable with only an input file
in which case it just prints stuff and _there is no output file_.
This could be changed if required. The current print routine is
based on the old (Zender) hyperslabbing algorithm (and printing
fails for wrapped coordinates I think). So printing directly from
nco_msa_rec_clc() sounds like a fine approach with no significant
downside.

I/O speed is not an issue when printing, so as many disk accesses
as are required are fine. I assume humans are reading the printed
output interactively and no one prints gigabytes of data (but they
do hyperslab and average that much). Thus as long as text scrolls
by on your screen at a rate faster than you can read it does not
matter how slow the printing algorithm is.

> Caching indices :-
> The factors which I think determine the peformance of
> nco_msa_rec_clc() when multi-slabbing are :-
> (1) The output dimension size
> (2) The total number of slabs.
> (3) The original dimension size.

> My idea is to cache say 1000 slab indices :-
> In certain cases it will not make a difference e.g when we have
> interlaced slabs -d time,,,3 -d time,,,4
> and the input dimension size is very large

Are you suggesting caching to reduce disk I/O or index computation?
Saving wallclock time and reducing redundant computations is
always a good idea but I have no sense of how many disk accesses your
algorithm generates. I'm not worried so much about how often indices
are (re-)computed, since computations are so much faster than disk
I/O.

You never sent the specifics of your newest multi-slabbing algorithm.
I just posted the description of your first version.
You said you had changed it a little to incorporate some aspects of my
algorithm. When you have time, please describe the final algortithm
in the thread on the developer forum so others can learn from it.

Thanks,
Charlie