Re: contribution and cvs

[Jason Stover]

'sorry for the belated response. My semester
started last week. 

On Sun, Aug 21, 2005 at 02:39:16PM -0700, Ben Pfaff wrote:

> Looking at the structures in cat.h, I have some questions.   First, I
> *think* that each row in the `struct recoded_categorical' matrix `m'
> has nonzero values only along a diagonal.  Is that true?  The big
> header comment says that a variable might map to (0 1 0 1 1), and I
> don't see how that can happen.

It can't happen now. There may be occasion in the future for
that kind of coding. The real reason the comment says that is
just that I never fixed it.

> 
> The comment on `m' in `struct recoded_categorical' refers to "hash a".
> I don't see any hashes around.

I had a hash in cat.c originally, but dropped it and never fixed
the comment.

> 
> Should the definition of `struct recoded_categorical' be in
> cat.h?  I only see a few references to its internals from outside
> cat.c.  It might be better to define accessor functions for those
> purposes and move the definition to cat.c.  I don't know whether
> this applies to other structs in cat.h.

Probably not. I didn't think that of until you mentioned it.

> 
> The extensive comment on var_cols in `struct design_matrix' makes me
> wonder whether an array of structs would be better, e.g.
>     struct design_matrix_var 
>      {
>        int first_column;            /* First column (see recoded_categorical
>                                      struct for full info). */
>        struct variable *var;        /* Variable. */
>      };

I think an array of such structs would be better than the var_cols array
in there now.

> 
> Next, some general comments.
> 
> Mechanics:
> 
(...mucho comments de Ben on mechanics, style )

I'll change those things.

> > I'm not happy with the categorical recode routines, and I don't
> > think I ever will be. I suspect a less offensive way to implement
> > them would be to put the struct recoded_categorical inside a struct
> > variable, but this wouldn't eliminate the need for some of the
> > ugly code in cat.c.
> 
> I think that you might have missed one of the mechanisms that
> PSPP has for adding `hooks' into `struct variable'.  The `aux'
> and `aux_dtor' members of `struct variable' are set aside for
> procedures to tag variables with auxiliary data.  The idea is
> that you attach a structure with var_attach_aux(), then access it
> as needed.  Later you can clear the aux data with
> var_clear_aux(), or you can clear it from the entire dictionary
> with dict_clear_aux().

I'll look into this. I didn't do it before because of lack of familiarity
with PSPP internals and fear of stomping on those internals. But
this recoding needs to be done in a less klugey way. 

-Jason

> -- 
> Ben Pfaff 
> email: address@hidden
> web: http://benpfaff.org

-- 
address@hidden
SDF Public Access UNIX System - http://sdf.lonestar.org