What is wrong with this Perl subroutine?

Question

I'm trying to implement a subroutine that calculates the d-neighbors of an input string. This is apart of an implementation of planted motif search, but my question is much more general. Here is the code:

#subroutine for generating d-neighbors
sub generate_d_neighbors{
    # $sequence is the sequence to generate d-neighbors from
    # $HD is the Hamming Distance
    my ($sequence, $HD) = @_;

    for(my $i = 0; $i=$HD; $i++){
        my @l = ['A', 'C', 'T', 'G'];
        my @t = splice(@l,$sequence[$i]);  
       #TODO
    }
}

The error is occurring at the last line, saying that:

Global symbol "@sequence" requires explicit package name (did you forget to declare "my @sequence"?

It was my understanding that Perl does not take parameters in the form subroutine(param1, param2) like in Java for example, but why is $sequence not being recognized as already having been initialized?

You use `$sequence[0]`, but you have not declared `@sequence`. In Perl, `$sequence` and `@sequence` are two different variables — Håkon Hægland, Apr 21 '17 at 14:26
Also, the assignment `$i=$HD` is probably not what you meant. Normally some kind of comparison goes there. — , Apr 21 '17 at 14:27
`$sequence[0]` is how you access an element of an array named `@sequence`. Perl correctly notes that it doesn't know anything about a variable named `@sequence`. You are likely passing an array *reference*, which should be accessed with a dereferencing arrow: `$sequence->[0]` — Hunter McMillen, Apr 21 '17 at 14:27

Sinan Ünür · Answer 1 · 2017-04-21T15:04:33.003

4

There are some problems with your code:

sub generate_d_neighbors{
    my ($sequence, $HD) = @_;

    for(my $i = 0; $i=$HD; $i++){
        my @l = ['A', 'C', 'T', 'G'];
        my @t = splice(@l,$sequence[$i]);  
    }
}

First, let's look at

    for(my $i = 0; $i=$HD; $i++){

Assuming $HD is nonzero, this loop will never terminate because the condition will never be false. If you wanted $i to range from 0 to $HD, writing the statement as for my $i (0 .. $HD) would have been better.

Second, you have

        my @t = splice(@l,$sequence[$i]);

where you seem to assume there is an array @sequence and you are trying to access its first element. However, $sequence is a reference to an array. Therefore, you should use

$sequence->[$i]

Third (thanks @Ikegami), you have

        my @l = ['A', 'C', 'T', 'G'];

in the body of the for-loop. Then @l will contain a single element, a reference to an anonymous array containing the elements 'A', 'C', 'T', and 'G'. Instead, use:

my @l = qw(A C T G);

I am not sure exactly what you want to achieve with splice(@l, $sequence->[$i]), but that can be better written as:

 my @t = @l[0 .. ($sequence->[$i] - 1)];

In fact, you could reduce the two assignments to:

 my @t = qw(A C T G)[0 .. ($sequence->[$i] - 1)];

edited Apr 21 '17 at 15:04

answered Apr 21 '17 at 14:51

Sinan Ünür

116,958
15
196
339

@ikegami Thank you for pointing those out. I am not sure if the usage of `splice` in the original snippet is what the OP intended, but taking it at face value, I tried to incorporate your feedback. – Sinan Ünür Apr 21 '17 at 15:06
The splice line was intended to remove the base at the current position. Example, `$sequence = "ATG";` and `$HD = 1;` At position 0, I want to remove `A`, and be left with just `C T G`. Similarly for positon 1, remove `T` and be left with `A C G`, etc. – Kyle Weise Apr 21 '17 at 15:21
So, is `$sequence` a simple scalar or a reference to an array? – Sinan Ünür Apr 21 '17 at 15:57
@SinanÜnür a reference to an array, yes. So that I'll be able to input a sequence (array) into `generate_d_neighbors` and replace each position's letter of the sequence with the 3 other possible letters. – Kyle Weise Apr 21 '17 at 16:17
`splice(@x, 2)` deletes everything from index 2 on. It seems you need `grep`. – Sinan Ünür Apr 21 '17 at 20:05

score 0 · Answer 2 · answered Apr 21 '17 at 14:43

0

It looks to me like you want

substring($sequence, 0, 1)

instead of

$sequence[0].

In Perl, strings are first class variables, not a type of array.

Or maybe you want splice(@l, $sequence->[0])?

answered Apr 21 '17 at 14:43

ysth

96,171
6
121
214

Whoops, just edited the question. Meant to be `$i` and no 0 – Kyle Weise Apr 21 '17 at 14:47

score 0 · Answer 3 · answered Apr 21 '17 at 15:03

This list-assignment syntax:

my (@sequence, $HD) = @_;

doesn't do what you might want it to do (put the last argument in $HD and the rest in @sequence). The array always takes all the arguments it can, leaving none for whatever comes after it.

Reversing the order can work, for cases where there is only one array:

my ($HD, @sequence) = @_;

and you make the corresponding change in the caller.

To solve the problem more generally, use a reference:

my ($sequence, $HD) = @_;

and call the sub like this:

generate_d_neighbors(\@foo, $bar);

or this:

# Note the brackets, which make an array reference, unlike parentheses
# which would result in a flat list.
generate_d_neighbors([...], 42);

If you use a prototype:

sub generate_d_neighbors (\@$)

then the caller can say

generate_d_neighbors(@foo, $bar);

and the @foo automatically becomes a reference as if it had been\@foo.

If you use any of the reference-based solutions, you must alter the body of the function to use $sequence instead of @sequence, following these rules:

Change @sequence to @$sequence
Change $#sequence to $#$sequence
Change $sequence[...] to $sequence->[...]
Change @sequence[...] to @$sequence[...] (but be sure you really meant to use an array slice... if you're new to perl you probably didn't mean it, and should have used $sequence[...] instead)

What is wrong with this Perl subroutine?

3 Answers3

Linked