On Thu, Feb 4, 2010 at 2:24 AM, John W. Krahn <jwkr...@shaw.ca> wrote:
> Eric Mooshagian wrote:
>
>> Dear All,
>>
>
> Hello,
>
Hi John, Thanks for the notes.
>
>
I have a few subroutines that I use to first build an index for several
>> arrays and then, for example, take the mean for the index values. When I
>> build the index I can exclude particular values in an array as follows:
>>
>> my $index = defindex(
>> exclude => ["0",\...@accuracy],
>> valueof => [...@subject,\...@side,\...@valence],
>> );
>>
>>
>> This works fine, but only only for excluding single values like (e.g,
>> "0"). What I would really like to do is pass an argument that includes
>> comparators, for example, in pseudo code:
>>
>> exclude => (responsetime <= 200) && (responsetime >= 1200) where
>> responsetime refers to an array.
>>
>> However, I have no idea how to do this using named parameters. Is there a
>> strategy for passing such arguments to a subroutine?
>>
>
> Like Perl's built-in sort() function or the File::Find::find() function you
> can use a subroutine reference, for example, in pseudo code:
>
> exclude => sub { return something if (responsetime <= 200) && (responsetime
> >= 1200) }
Thanks! I think this is the clue I've been looking for though I don't yet
understand how to implement the code. Are dispatch tables relevant here?
All other mistakes below were purely out of naivete. Thank you for the
changes.
>
> My current code:
>>
>> use strict;
>> use warnings;
>> use List::Util qw(sum);
>> use List::MoreUtils qw(indexes each_arrayref);
>>
>> #sample data
>> my @subject = qw(Sub1 Sub1 Sub1 Sub1 Sub1 Sub1 Sub1 Sub1 Sub2 Sub2 Sub2
>> Sub2 Sub2 Sub2 Sub2 Sub2);
>> my @side= qw(left left left left right right right right left left left
>> left right right right right);
>> my @updown= qw(up down down up up up down down up up down down up up down
>> down);
>> my @valence= qw(minus plus minus plus minus plus minus plus minus plus
>> minus plus minus plus minus plus);
>> my @responsetime=
>> (999.4,233.1,456.1,763.55,241.1,742.67,223.78,555.89,664.91,157.11,748.12,
>> 848.13, 253.14, 623.15, 635.16, 423.17);
>> my @accuracy= qw(1 1 0 1 0 1 1 0 1 0 1 0 0 1 1 1);
>>
>>
>> ###########################################
>> my $index = defindex(
>> exclude => ["0",\...@accuracy],
>> valueof => [...@subject,\...@side,\...@valence],
>> );
>>
>> my @index = @$index;
>>
>> getmean(\...@index,\...@responsetime);
>>
>> ######################################
>> #Code for the module
>>
>> #define the index
>> sub defindex {
>> my $i;
>> my $j;
>> my @combined;
>> my @meshed;
>> my @index;
>> my @match;
>> my $exclude_input;
>> my @values;
>> my $input;
>> my @exclude_values;
>>
>
> It is usually better to define variables where they are first used instead
> of all at the start of code.
>
>
> #Get the input arguments
>> my (%options) = @_;
>> $input = $options{valueof};
>> $exclude_input = $options{exclude};
>> #Put the inputs into arrays
>> @values = @$input if $input || die "No input to defindex!\n";
>> @exclude_values = @$exclude_input if $exclude_input;
>>
>
> You should validate subroutine input at the point of input:
>
> @_ and not @_ % 2 and my %options = @_ or die "Invalid input to
> defindex!\n";
> my @values = @{ $options{ valueof } };
> my @exclude_values = @{ $options{ exclude } };
>
>
> #Number of arguments for the index
>> my $num_args = (scalar(@values));
>>
>
> The expression (scalar(@values)) is already in scalar context because the
> left-hand side of the assignment forces scalar context.
>
>
> for my $i (0..($num_args-1)) {
>> push(@combined,$values[$i]);
>> }
>>
>
> That is usually written as:
>
> for my $i ( 0 .. $#values ) {
> push @combined, $values[ $i ];
> }
>
> Or simply:
>
> my @combined = @values;
>
> @meshed = mesh(@combined);
>>
>
> But you don't really need @combined, you could just use @values instead.
>
> my @meshed = mesh( @values );
>
>
> #Get the indices of the excluded values
>> my $exclude = exclude(@exclude_values);
>> my @exclude_indices = @$exclude;
>>
>> $j=0;
>> while (@meshed) {
>> my $match = grep /^$j$/, @exclude_indices;
>> unless ($match) {
>> $index[$j]= join('', splice(@meshed,0,$num_args));
>> $j++;
>> } else {
>> my $ignore = join('', splice(@meshed,0,$num_args));
>>
>
> The $ignore variable is not used anywhere so you don't really need it. The
> only required part is the splice() function.
>
>
> splice @meshed, 0, $num_args;
>
> $index[$j]= "_";
>> $j++;
>> }
>>
>
> Your loop might be better written as:
>
> my $j = 0;
> my @index;
> while ( @meshed ) {
> my $temp = join '', splice @meshed, 0, @values;
>
> $index[ $j++ ] = grep( $_ == $j, @exclude_indices ) ? '_' : $temp;
> }
>
> undef my(@match);
>>
>
> This is a superfluous statement, it can be safely removed.
>
>
> }
>> my @filtered_idx = grep($_ ne "_",@index);
>> return \...@filtered_idx;
>>
>> } #end defindex
>>
>>
>> sub exclude {
>> my @values;
>> my @match_idx;
>> my @uniq_idx;
>> my $i;
>> my $j;
>>
>
> Again, it is usually better to define variables where they are first used
> instead of all at the start of code.
>
>
> while (@_) {
>> my ($key, $values_ref)=(shift, shift);
>>
>
> I usually like to write that as:
>
> my ($key, $values_ref) = splice @_, 0, 2;
>
> As it is less ambiguous.
>
> @values = @$values_ref;
>>
>
> Why not use $values_ref directly instead of making a copy?
>
>
> for ($i = 0; $i < @values; $i++) {
>>
>
> That is usually written as:
>
> for my $i ( 0 .. $#values ) {
>
>
> if ($values[$i] eq $key) {
>> push(@match_idx, $i); # save the index
>> }
>> }
>>
>
> You could always use grep() instead of a for loop:
>
> push @match_idx, grep $key eq $values_ref->[ $_ ], 0 ..
> $#$values_ref;
>
>
> }
>> #find the unique elements
>> my %seen = ();
>> foreach $j (@match_idx) {
>>
>
> foreach my $j (@match_idx) {
>
> @{$_} = split /\s/, $j;
>>
>
> That statement is superfluous and can be safely removed. (And why are you
> modifying a global variable anyway?) (And how would $j ever contain any
> whitespace?)
>
>
> push(@uniq_idx, $j) unless $seen{$j}++;
>> } #foreach $a
>>
>
> That is usually written as:
>
> #find the unique elements
> my %seen;
> my @uniq_idx = grep !$seen{ $_ }++, @match_idx;
>
>
> return \...@uniq_idx;
>>
>> } #end exclude
>>
>> # From Shlomi Fish
>> sub get_combos_from_kv {
>> my ($keys, $values) = @_;
>>
>> my %combos;
>> foreach my $i (0..$#$keys) {
>> my $key = $keys ->[$i];
>> my $value = $values ->[$i];
>>
>> push (@{$combos{$key}},$value);
>> }
>> return \%combos;
>> }
>>
>> # get the mean by index
>> sub getmean {
>> my @ke...@{$_[0]};
>> my @valu...@{$_[1]};
>> my $combos = get_combos_from_kv(\...@keys,\...@values);
>> my %combos = %$combos; #dereferenced
>> print "ID\tMeans\n";
>> my $key;
>> my $mean;
>> foreach $key (sort keys %combos) {
>> my $mean = (sum(@{$combos{$key}}))/(scalar(@{$combos{$key}}));
>> printf "$key\t%.3f\n", $mean;
>> }
>>
>
> Why all the dereferencing, copying and referencing and the redundant use of
> scalar()?
>
> my $combos = get_combos_from_kv( @_ );
>
> print "ID\tMeans\n";
> for my $key ( sort keys %$combos ) {
> printf "$key\t%.3f\n", sum( @{ $combos->{ $key } } ) / @{ $combos->{
> $key } };
>
> }
>
> } #end of getmean
>>
>> #borrowed from the List::MoreUtils Module by Tassilo von Parseval
>> sub mesh
>> (\...@\@;\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@\...@\@)
>> {
>>
>
> That should be written as:
>
> sub mesh {
>
> The prototype doesn't appear to be doing what you seem to think it is
> doing.
>
>
> my $max = -1;
>> $max < $#$_ && ($max = $#$_) for @_;
>>
>> map { my $ix = $_; map $_->[$ix], @_; } 0..$max;
>> }
>>
>
>
>
> John
Thanks,
Eric
>
