Re: How can I improve the efficiency of this very intensive code?

Yes you can do quite a few things to that sub:

sub IsStrongMatch
{
    # Return true if id2 is only top ranked match for id1
    my $id1 = shift;
    my $id2 = shift;
    my $rC  = shift;

    for my $i1 ( keys %{$rC} )
    {
        next if $i1 == $id1;
        foreach my $i2 ( sort { $rC->{$i1}->{$a} <=> $rC->{$i1}->{$b} 
+} keys %{$rC->{$i1}} )
        {
            if ( $id2 == $i2 )
            {
                return 0;
            }
            last;
        }
    }
    return 1;
}
[download]

A. You can remove the temporary variable $i2, it isn't used anywhere in a subscope and $_ can be used instead, that way you don't have to copy to a temp var.
B. You can use @_[0-2] rather than shifting to temp variables, which is many more processes than needed.
C. You can return undef rather than 0 rumor mill says this is a minor optimization.

Evan Carroll
www.EvanCarroll.com

Comment on Re: How can I improve the efficiency of this very intensive code? Select or Download Code

Replies are listed 'Best First'.
Re^2: How can I improve the efficiency of this very intensive code? by EvanCarroll (Chaplain) on Aug 06, 2005 at 22:33 UTC
`sub IsStrongMatch { # Return true if id2 is only top ranked match for id1 my $id1 = shift; my $id2 = shift; my $rC = shift; for my $i1 ( keys %{$rC} ) { next if $i1 == $id1; foreach my $i2 ( sort { $rC->{$i1}->{$a} <=> $rC->{$i1}->{$b} +} keys %{$rC->{$i1}} ) { if ( $id2 == $i2 ) { return 0; } last; } } return 1; }` [download] On second look, I have to wonder what the purpose of the '`last`' is being in a for loop, outside of a conditional. If that is what you wanted you could just take the first element of the array returned to by sort, and run the if {} on that. Evan Carroll www.EvanCarroll.com	[reply] [d/l] [select]
Re^3: How can I improve the efficiency of this very intensive code? by mrborisguy (Hermit) on Aug 06, 2005 at 23:16 UTC
Update: I just read frodo72's post, which makes the point better than this code. In that case, if you are just going to use the first one, wouldn't finding the first one be faster than `sort`? `sub IsStrongMatch { # Return true if id2 is only top ranked match for id1 my $id1 = shift; my $id2 = shift; my $rC = shift; for my $i1 ( keys %{$rC} ) { next if $i1 == $id1; my $i2; for ( keys %{ $rC->{ $i1 } } ) { $i2 = $_ if not defined( $i2 ); # if there's a good way to find a starting $i2 # before beginning, then take this out, and # don't check every time $i2 = $_ if $rC->{ $i1 }->{ $_ } < $rC->{ $i1 }->{ $i2 }; } if ( $id2 == $i2 ) { return 0; } } return 1; }` [download] -Bryan	[reply] [d/l] [select]
Re^4: How can I improve the efficiency of this very intensive code? by polettix (Vicar) on Aug 07, 2005 at 00:07 UTC
This is exactly what I was going to suggest, apart from a minor optimisation regarding the initialisation of the first value. The I recalled of the existence of `reduce` in List::Util, which does the job nicely. But maybe I should have been less cryptic and explain what the solution does exactly - thanks for the example, I'll update my post linking to it :) I know it's a corner case that should not happen, but I'd check for `$i2` to be defined before comparing it against `$id2`. Flavio perl -ple'$_=reverse' <<<ti.xittelop@oivalf Don't fool yourself.	[reply] [d/l] [select]
Re^3: How can I improve the efficiency of this very intensive code? by clearcache (Beadle) on Aug 06, 2005 at 22:37 UTC
Yes, certainly. Thank you.	[reply]


Think about Loose Coupling
	PerlMonks