Re: ROC is ready for review

[Ben Pfaff]

John Darrington <address@hidden> writes:

> I think the ROC procedure is finished.
>
> Any reviews, before I merge to master are welcome.

In the documentation, a '=' is missing from the syntax summary
for the PLOT subcommand.

"consolidate" is spelled with an "i" :-)

casereader_create_distinct seems to misuse the return value of
casereader_get_proto.  It assigns this (const) return value to a
non-const variable, then modifies it (sometimes).  To modify it,
and to keep a reference to it in cdr->proto, it needs its own
reference to it (with caseproto_ref).  Then it needs to release
that reference (with caseproto_unref) when cdr is destroyed.

I don't really understand the algorithm used by
casereader_create_distinct.  Why are there two layers
(casereader_create_filter_func and casereader_create_translator)?
Why is it necessary to use casereader_peek (which forces all of
the input data to stay around) instead of reading cases one at a
time (and possibly being able to throw away data that has been
consumed)?

cmd_roc returns a literal value of 2 on error.  Probably it
should return CMD_FAILURE instead (which has value -2).

cmd_roc returns a literal value of 1 on success.  Probably it
should return CMD_SUCCESS instead (which does have value 1).

cmd_roc fails to free allocated memory (e.g. cmd.vars) on some
errors.

accumulate_counts will skip the first case if its cp value is
SYSMIS.  I don't know whether that is important.

process_group does a *lot* of manipulation of cases and
caseprotos and readers and writers.  I didn't check it in detail
because it wasn't obvious to me what the overall goal was.  It
could really use a high-level comment.

It seems that there should be macros for 3 and 4 (following
VALUE, N_EQ, N_PRED)?  The literal values 3 and 4 are used in a
number of places.

The do_roc function could use some internal comments, that
describe at least what each block of code is doing.

roc.h doesn't declare anything that one can't get from command.h,
so I would delete it.  Besides, command.c doesn't include roc.h,
so the prototype in roc.h doesn't provide the assurance that a
prototype should (of making sure that all the callers see the
correct function signature).

I didn't review the correctness of the algorithms at all (nor am
I familiar with what algorithms should be used).
-- 
"Then, I came to my senses, and slunk away, hoping no one overheard my
 thinking."
--Steve McAndrewSmith in the Monastery