Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

Prob with 'while' loop and subroutine calling

by cool (Scribe)
on Aug 18, 2006 at 18:39 UTC ( #568238=perlquestion: print w/replies, xml ) Need Help??

cool has asked for the wisdom of the Perl Monks concerning the following question:

Hi Monks!

Below is the code that generates jpg image using GD package. This is written by some previous student of my lab. Pl pardon me as this code is without any '-w' and 'use strict'. I want to use this for my analysis. I want to write it properly but got stuck in the sub part. How the while in the subroutine is working? I am not getting how the while loop is executing for complete arrays and how subrouteing is just graph() without passing any value. I really had a tough time to upgrade the code to use 'use strict'. So, can any monk would suggest me any better way to write this code that is more robust, with use strict'. Thank you for taking out time to read it.

Input files are also there with the code.

#!/usr/bin/perl use GD; #use strict; my @x1; my @y1; my $max1; my $max2; my $min1; my $min2; my $i; open(FILE1,"fwd.stb"); while(<FILE1>) { my @value = split(/\s+/,$_); $x1[$i] = $value[0]-7; $y1[$i] = $value[1]; if($i != 0 && $y1[$i] > $max1) { $max1 = $y1[$i]; } if($i != 0 && $y1[$i] < $min1) { $min1 = $y1[$i]; } if($i == 0) { $min1 = $max1 = $y1[$i]; } $i++; } my $lines = $i; # Define constants my $X1start = 50+20; # X-axis my $Y1start = 300; # X-axis my $X1end = 450+$lines-400; # X-axis my $Y1end = 300; # X-axis my $X2start = 50+20; my $Y2start = 100; my $X2end = 50+20; my $Y2end = 300; # create a new image my $im = new GD::Image($X1end+50,$Y2end+100); # allocate some colors my $white = $im->colorAllocate(255,255,255); my $green = $im->colorAllocate(0,255,0); my $black = $im->colorAllocate(0,0,0); my $red = $im->colorAllocate(255,0,0); my $blue = $im->colorAllocate(0,0,255); # set the caption for the graph $im->string(gdLargeFont,int($lines/2)-50,20,"ENERGY GRAPH",$black) +; # draw the y-axis & x-axis $im->line($X2start,$Y2start,$X2end,$Y2end,$black); $im->line($X1start,$Y1start,$X1end,$Y1end,$black); # set the caption for x-axis and y-axis $im->string(gdLargeFont,int($lines/2)-50,335,"NUCLEOTIDE POSITION" +,$black); $im->stringUp(gdLargeFont,5,260,"FREE ENERGY(delta G)",$black); $im->string(gdSmallFont,int($lines/2)+100,20," Genomic Sequence - +------",$red); $im->string(gdSmallFont,int($lines/2)+100,40," Shuffled Sequence - +------",$blue); open(FILE2,"total_shuffle.stb"); $i = 0; my @x2; my @Y2; my $min; my $max; while(<FILE2>) { my @value = split(/\s+/,$_); $x2[$i] = $value[0]-7; $y2[$i] = $value[1]; if($i != 0 && $y2[$i] > $max2)if($i != 0 && $y2[$i] > $max2) { $max2 = $y2[$i]; } if($i != 0 && $y2[$i] < $min2) { $min2 = $y2[$i]; } if($i == 0) { $min2 = $max2 = $y2[$i]; } $i++; } if($min1 <= $min2) { $min = $min1; } else { $min = $min2; } if($max1 >= $max2) { $max = $max1; } else { $max = $max2; } @y = @y1; @x = @x1; graph(); @y = @y2; @x = @x2;my $png_data = $im->png; open (DISPLAY,"| display -") || die; #open (DISPLAY,">image.jpg") || die; binmode DISPLAY; print DISPLAY $png_data; close DISPLAY; sub graph { # set min and max energy my $minEnergy = $min; my $maxEnergy = $max; # set the length of x-axis and y-axis my $xLen = $X1end-$X1start; my $yLen = $Y2end-$Y2start; # Scaling the values $Yscale = $yLen/(abs($minEnergy)-abs($maxEnergy)); $scale = ($minEnergy-$maxEnergy)/5; $incEnergy = $maxEnergy; while($incEnergy > $minEnergy) ##### Prob loop#### { $x1 = $X1start-2; $y1 = int(((abs($incEnergy)-abs($maxEnergy))*$Yscale)+ +$Y2start); $x2 = $X1start+2; $im->line($x1,$y1,$x2,$y1,$black); $displayEnergy = sprintf("%2.1f",$incEnergy); $im->string(gdSmallFont,$x1-35,$y1-10,$displayEnergy,$ +black); $incEnergy = $incEnergy+$scale; } # set the length of nucleotide $len = @x; # mark 10 points on the position axis $posSkip = int(@x/10); # If the length of the fragment is lesser than the x-axis $skipPixel = $xLen/$len; for($i=0;$i<$len-1;$i++) { $x1 = int(($i*$skipPixel)+$X1start); $y1 = int(((abs($y[$i])-abs($maxEnergy))*$Yscale)+$Y2s +tart); $x2 = int((($i+1)*$skipPixel)+$X1start); $y2 = int(((abs($y[$i+1])-abs($maxEnergy))*$Yscale)+$Y +2start); if($call == 0) { $im->line($x1,$y1,$x2,$y2,$red); } else { $im->line($x1,$y1,$x2,$y2,$blue); } $count++; if($count == $posSkip) { $x1 = int(($i*$skipPixel)+$X1start); $y1 = $Y1end-2; $x2 = int(($i*$skipPixel)+$X1start); $y2 = $Y1end+2; if($call == 0) { $im->line($x1,$y1,$x2,$y2,$black); $im->string(gdSmallFont,$x1,$y1+10,$i+ +1,$black); } $count = 0; } } $call++; }

File 'fwd.stb' and file 'total_shuffle.stb'

8 -19.74 9 -19.89 10 -19.91 11 -18.67 12 -18.27 13 -17.55 14 -16.71 15 -16.29 16 -16.59 17 -16.99 18 -16.82 19 -16.4 20 -16.84 21 -18.13 22 -19.37 23 -19.76 24 -19.91 25 -20.19 26 -20.99 27 -20.82 28 -21.27 29 -22.63 30 -23.17 31 -22.78 32 -22.5 33 -23.36 34 -23.76 35 -23.76 36 -22.96 37 -22.57 38 -22.96 39 -23.92 40 -23.52 41 -24.41 42 -24.8 43 -23.86 44 -23.3 45 -24.09 46 -24.54 47 -24.38 48 -24.38 49 -23.65 50 -23.66
8 -20.496 9 -20.986 10 -21.534 11 -21.652 12 -21.77 13 -22.184 14 -22.024 15 -21.546 16 -21.298 17 -21.116 18 -20.928 19 -21.054 20 -20.728 21 -20.394 22 -20.212 23 -19.808 24 -19.346 25 -19.42 26 -19.47 27 -19.282 28 -19.37 29 -19.624 30 -19.916 31 -19.874 32 -20.24 33 -20.22 34 -20.394 35 -20.89 36 -21.142 37 -21.192 38 -21.432 39 -21.448 40 -21.08 41 -20.98 42 -21.252 43 -21.446 44 -21.2 45 -21.29 46 -21.556 47 -21.956 48 -22.03 49 -21.562 50 -21.424

Replies are listed 'Best First'.
Re: Prob with 'while' loop and subroutine calling
by imp (Priest) on Aug 18, 2006 at 18:57 UTC
    Ahh the joys of inheriting code.

    The reason why graph() works is that most of the variables in that script are being treated as globals, which is a bad practice as it makes it very difficult to debug.

    Everything but $minEnergy, $maxEnergy, $xYen,and $yLen are used globally within graph(). Some of these are forgiveable, as they are being treated as constants.. but anything that could be modified should be passed to the graph() sub. This includes the $im object.

    I would strongly advise enabling strict and warnings. It will tell you all of the places where these variables are being used inappropriately.

    It would also be good to split some of the behaviour into worker functions that encapsulate one set of behaviour.

      Thanks for your input monk. I will try to implement and then get back to you :)
Re: Prob with 'while' loop and subroutine calling
by GrandFather (Sage) on Aug 18, 2006 at 20:04 UTC

    The edited version of the code contains ## comments where I made changes or inserted comments.

    Note in particular that the array @Y2 was renamed to @y2! That (presumed) typo was picked up by use strict;.

    I don't see any particular issue with the while loop. Perhaps you could describe the problem that you are having with it?

    Update bugs fixed - see replies


    DWIM is Perl's answer to Gödel
      Dear GrandFather,

      First of all, thanks for taking so much pain for my problem.

      In the code I posted, mistakenly graph()(line 126) left out while pasting.

      The code I have pasted is working and giving results. But without 'use warning' I don feel confident. And also global variable of this code are interfering with rest of my code(actually this piece is part of a large work).

      For whileloop case, I didn't get, what is prompting it to run over complete array. like it is not written as while (@x) or while ($fh)

      So what I think is, this should run only once when it enters in the sub; when for the first time control enters the loop while($incEnergy > $minEnergy)What is prompting it to return? I know its something v silly that I am not getting, but pl guide me.

      Can you pl explain me how this part of your code is working!! Just briefly, what is the concept behind or some link.. Also while compiling its showing this line

      Global symbol "$x1" requires explicit package name at monk_graph.pl line 150.

      if(@$x1) { $$max = $y->[-1] if $y->[-1] > $max; $$min = $y->[-1] if $y->[-1] < $min; } else { $$min = $$max = $y->[-1];}

        while ($incEnergy > $minEnergy)
        In that while loop there is:
        $incEnergy = $incEnergy+$scale;
        So presumably, incEnergy will grow until it is larger than minEnergy(update: incEnergy will shrink until its less than minEnergy), and the while loop will terminate eventually if $scale is greaterless than zero. But you have:
        $scale = ($minEnergy-$maxEnergy)/5;
        Which is greater? min or max? Normally I'd think the max would be greater, so scale would be negative, and you may have an infinite loop. If the code is correct, then it is confusing. Nevermind. But at least it answers your question on how the loop terminates.
        Global symbol "$x1" requires explicit package name at monk_graph.pl line 150.
        I assume that is supposed to be "@x", not "@$x1" in the code.

        The while loop decrements $incEnergy from $maxEnergy downward in steps of $scale (which is negative) until $incEnergy is less than $minEnergy. It looks fine to me, although it may be a good place for a C type for loop:

        for ($incEnergy = $maxEnergy; $incEnergy > $minEnergy; $incEnergy += $ +scale)

        and omit the $incEnergy = $incEnergy+$scale; at the end of the loop. It looks like the while loop is generating points along an energy axis.

        if(@$x1) { should be if(@$x) {. The test checks to see if @$x contains any elements.

        The LoadFile sub takes a number of reference parameters. The first line in the sub:

        my ($filename, $x, $y, $min, $max) = @_;

        declares the variables and sets them to the parameter values. The last four variables are references. So $$max is the scalar variable that $max refers to. $x and $y are references to arrays and $y->[-1] is the last element of the array refered to by $y. So the line:

        $$max = $y->[-1] if $y->[-1] > $$max;

        means assign the last element of @$Y (the array that $Y refers to) to the scalar that $max refers to if the element is greater than the current maximum. (Note there was an error in this code. See update in previous post.)


        DWIM is Perl's answer to Gödel
Re: Prob with 'while' loop and subroutine calling
by jwkrahn (Monsignor) on Aug 18, 2006 at 22:38 UTC
    So, can any monk would suggest me any better way to write this code that is more robust, with use strict'.
    This produces the same output as the original and works with warnings and strict enabled:

    Update: fixed an edge case in sub read_file.

    #!/usr/bin/perl use warnings; use strict; use GD; my ( $min1, $max1, $x1, $y1, $lines ) = read_file( 'fwd.stb' ); # Define constants my $X1start = 50 + 20; # X-axis my $Y1start = 300; # X-axis my $X1end = 450 + $lines - 400; # X-axis my $Y1end = 300; # X-axis my $X2start = 50 + 20; my $Y2start = 100; my $X2end = 50 + 20; my $Y2end = 300; # create a new image my $im = new GD::Image( $X1end + 50, $Y2end + 100 ); # allocate some colors my $white = $im->colorAllocate( 255, 255, 255 ); my $green = $im->colorAllocate( 0, 255, 0 ); my $black = $im->colorAllocate( 0, 0, 0 ); my $red = $im->colorAllocate( 255, 0, 0 ); my $blue = $im->colorAllocate( 0, 0, 255 ); my $half_lines = int( $lines / 2 ); # set the caption for the graph $im->string( gdLargeFont, $half_lines - 50, 20, 'ENERGY GRAPH', $black + ); # draw the y-axis & x-axis $im->line( $X2start, $Y2start, $X2end, $Y2end, $black ); $im->line( $X1start, $Y1start, $X1end, $Y1end, $black ); # set the caption for x-axis and y-axis $im->string( gdLargeFont, $half_lines - 50, 335, 'NUCLEOTIDE POSITIO +N', $black ); $im->stringUp( gdLargeFont, 5, 260, 'FREE ENERGY(delta G)', $black ); $im->string( gdSmallFont, $half_lines + 100, 20, ' Genomic Sequence - +------', $red ); $im->string( gdSmallFont, $half_lines + 100, 40, ' Shuffled Sequence - +------', $blue ); my ( $min2, $max2, $x2, $y2 ) = read_file( 'total_shuffle.stb' ); my $min = $min1 <= $min2 ? $min1 : $min2; my $max = $max1 >= $max2 ? $max1 : $max2; graph( $x1, $y1, $min, $max ); open my $DISPLAY, '|-:raw', 'display', '-' or die "Cannot open pipe to + 'display' $!"; print $DISPLAY $im->png; close $DISPLAY or warn $! ? "Error closing 'display' pipe: $!" : "Exit + status $? from 'display'"; exit 0; sub read_file { my $file_name = shift; open my $fh, '<', $file_name or die "Cannot open '$file_name' $!"; my ( $min, $max, @x, @y ); while ( <$fh> ) { my @value = split; push @x, $value[ 0 ] - 7; push @y, $value[ 1 ]; if ( @y > 1 ) { $max = $y[ -1 ] if $max < $y[ -1 ]; $min = $y[ -1 ] if $min > $y[ -1 ]; } elsif ( @y == 1 ) { $min = $max = $y[ -1 ]; } } return $min, $max, \@x, \@y, $.; } sub graph { my ( $x_ref, $y_ref, $minEnergy, $maxEnergy ) = @_; # Scaling the values my $Yscale = ( $Y2end - $Y2start ) / ( abs( $minEnergy ) - abs( $m +axEnergy ) ); my $scale = ( $minEnergy - $maxEnergy ) / 5; my $incEnergy = $maxEnergy; while ( $incEnergy > $minEnergy ) { ##### Prob loop#### my $x1 = $X1start - 2; my $y1 = int( ( ( abs( $incEnergy ) - abs( $maxEnergy ) ) * $Y +scale ) + $Y2start ); my $x2 = $X1start + 2; $im->line( $x1, $y1, $x2, $y1, $black ); my $displayEnergy = sprintf '%2.1f', $incEnergy; $im->string( gdSmallFont, $x1 - 35, $y1 - 10, $displayEnergy, +$black ); $incEnergy = $incEnergy + $scale; } # mark 10 points on the position axis my $posSkip = int( @$x_ref / 10 ); # If the length of the fragment is lesser than the x-axis my $skipPixel = ( $X1end - $X1start ) / @$x_ref; my $count; for my $i ( 0 .. $#$x_ref - 1 ) { my $x1 = int( $i * $skipPixel + $X1start ); my $y1 = int( ( abs( $y_ref->[ $i ] ) - abs( $maxEnergy ) ) * +$Yscale + $Y2start ); my $x2 = int( ( $i + 1 ) * $skipPixel + $X1start ); my $y2 = int( ( abs( $y_ref->[ $i + 1 ] ) - abs( $maxEnergy ) +) * $Yscale + $Y2start ); $im->line( $x1, $y1, $x2, $y2, $red ); $count++; if ( $count == $posSkip ) { $x1 = int( $i * $skipPixel + $X1start ); $y1 = $Y1end - 2; $x2 = int( $i * $skipPixel + $X1start ); $y2 = $Y1end + 2; $im->line( $x1, $y1, $x2, $y2, $black ); $im->string( gdSmallFont, $x1, $y1 + 10, $i + 1, $black ); $count = 0; } } } __END__
      Hi jwkrahn!!

      Thanks a lot for the guidance. Actually after trying my best for a perticular problem, if I get to see code by monk I can really improve my style of writing and also doing things efficiently. To tell you the truth after joing this site I started adding use strict or let say error msgs to my scripts. And now I clearly understands the value of these imp things which freshers like me usually ignore.

      Thanks again :)
Re: Prob with 'while' loop and subroutine calling
by starbolin (Hermit) on Aug 18, 2006 at 21:15 UTC
    my $max1; my $max2; my $min1; my $min2; my $i;
    These, and others are all used uninitialized. That is what strict has been trying to tell you.
    my ($max1, $max2, $min1, $min2, $i) = (0,0,0,0,0, );
    Would be better but if I see variable names like that ( $x1, $x2 ) then I think something is wrong with the program structure. In this case the author has not used subroutines to factor out duplicate code so he resorted to overlapping variable names and cluttered up the namespace.

        open(FILE1,"fwd.stb"); should be:     open(FILE1,"fwd.stb") or die "Can\'t open file\n";

    The same errors are repeated later in the code because the same code is repeated. Use subroutines to replace repeated code. At least you won't have to fix every error twice.

       for($i=0;$i<$len-1;$i++)
    This will skip the last element of the array. The conditional should be <= or a more perlish: for(0 .. scalar @x -1)

    You have two variables $call and $count that are neither declared nor initialized. Strict has been trying to tell you that also.



    s//----->\t/;$~="JAPH";s//\r<$~~/;{s|~$~-|-~$~|||s |-$~~|$~~-|||s,<$~~,<~$~,,s,~$~>,$~~>,, $|=1,select$,,$,,$,,1e-1;print;redo}

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://568238]
Approved by GrandFather
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (4)
As of 2020-02-29 04:25 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    What numbers are you going to focus on primarily in 2020?










    Results (128 votes). Check out past polls.

    Notices?