4
\$\begingroup\$

I have written the script to run locally the needle program for a query against the subject files. It returns the highest percentage result file as a string. In addition it also converts to html file (write_html function).

Can you help me to improve the script?

subject_path = settings.MEDIA_ROOT+"subject_files/"
query_path = settings.MEDIA_ROOT+"query_file/"


def run_bug():
    """The script runs the needle program for a query file against all the subject files in a folder and keeps only
    the highest identity score alignments.
    """

    query_list_fastafiles, subject_list_fastafiles = start_function()
    files_ending_with_one = filter_files_ending_with_one(subject_list_fastafiles)

    for j in range(len(query_list_fastafiles)):
        initial = 0
        needle_alignment_result = ''

        #take the scaffold sequence one by one
        for i in range(len(files_ending_with_one)):

            #path of the files
            file1 = query_path+ordered_query_fastafiles[j]
            file1 = subject_path+files[i]
            needle_output = run_needle_two_sequences(file1, file2)

            if needle_output == None:
                return needle_alignment_result = needle_output
            else:
                #print(ordered_query_fastafiles[j],ordered_scaffold_fastafiles[i])
                identity_percentage, result_file_as_string = needle_output
                #print(identity_percentage, results)
                identity_percentage = float(identity_percentage)

                if identity_percentage > initial:
                #keeps the highest percentage result among the files
                    initial = identity_percentage
                    needle_alignment_result = result_file_as_string

    write_html(needle_alignment_result)
    return needle_alignment_result
\$\endgroup\$
  • 1
    \$\begingroup\$ Ah I see the comment on your answer- un-rolledback \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 16 '19 at 18:22
  • \$\begingroup\$ return needle_alignment_result = needle_output is not valid Python code. \$\endgroup\$ – Graipher Sep 17 '19 at 7:42
3
\$\begingroup\$

A very short review, as I don't have the time for a more extensive one.

Enumerate

Some things I see right off the bat is range(len()). Instead of writing this, consider using enumerate. This will allow you to work with any iterable, not just countable, indexable objects.

Unnecessary else

I see this piece of code next:

if needle_output == None:
    return needle_alignment_result = needle_output
else:
    ... code here ...

The else here is unnecessary, as you exit the function after returning in the if. Just move the code in the else right after the if, and remove the unnecessary else, like so:

if needle_output == None:
    return needle_alignment_result = needle_output
... code here ...

is None vs == None

Below is from an amazing StackOverflow answer.

A class is free to implement comparison any way it chooses, and it can choose to make comparison against None means something (which actually makes sense; if someone told you to implement the None object from scratch, how else would you get it to compare True against itself?).

Practically-speaking, there is not much difference since custom comparison operators are rare. But you should use is None as a general rule.

Updated Code

def run_bug():
    """
    The script runs the needle program for a query file against all the
    subject files in a folder and keeps only the highest identity score alignments.
    """
    needle_alignment_result = ''

    query_list_fastafiles, subject_list_fastafiles = start_function()
    files_ending_with_one = filter_files_ending_with_one(subject_list_fastafiles)

    for j, _ in enumerate(query_list_fastafiles):
        initial = 0

        # Take the scaffold sequence one by one
        for i, _ in enumerate(files_ending_with_one):

            # Path of the files
            file1 = query_path + ordered_query_fastafiles[j]
            file1 = subject_path + files[i]
            needle_output = run_needle_two_sequences(file1, file2)

            if needle_output is None:
                return needle_alignment_result == needle_output

            identity_percentage, result_file_as_string = needle_output
            identity_percentage = float(identity_percentage)

            # Keeps the highest percentage result among the files
            if identity_percentage > initial:
                initial = identity_percentage
                needle_alignment_result = result_file_as_string

    write_html(needle_alignment_result)
    return needle_alignment_result
\$\endgroup\$
  • 1
    \$\begingroup\$ enumerate is only called-for if you're using both the value and the index. In this case you only use the index, so the OP's solution is preferable. \$\endgroup\$ – Reinderien Sep 16 '19 at 18:25
2
\$\begingroup\$

It took me a few reads to understand what's going on here, but I think I do, now. You're best to separate this into at least two parts: one part that generates the percentage and alignment result, and one part that applies the max function to get the best percentage-result tuple. This can be done easily with a function that yields whenever it gets such a tuple.

After some investigation in chat, we've arrived at a prior-to hidden business requirement: query_list_fastafiles will only ever be a sequence of length 1. As such, you definitely shouldn't have a loop. Instead, you should probably assert len(query_list_fastafiles) == 1 and then continue on with the inner loop. Having an outer loop will simply discard all results for the inner loop except the last one.

\$\endgroup\$
  • \$\begingroup\$ Since each query file will have a needle alignment result. The case is (1) where this loop produce needle_alignment_result. \$\endgroup\$ – catuf Sep 16 '19 at 17:48
  • \$\begingroup\$ OK... but it doesn't do that. needle_alignment_result is overwritten in the inner loop. \$\endgroup\$ – Reinderien Sep 16 '19 at 17:50
  • \$\begingroup\$ You are right. The new needle_alignment_result should define inside the j loop. I did a mistake while copying. BTW: Just realized run_bug is a weird name. It has to be run_needle. \$\endgroup\$ – catuf Sep 16 '19 at 17:54
  • \$\begingroup\$ @catuf OK. Please edit your question to reflect your real code. \$\endgroup\$ – Reinderien Sep 16 '19 at 18:15
  • \$\begingroup\$ So, this is not an improvement. You're still running needle_alignment_result = result_file_as_string in the inner loop without a break. Think about the behaviour if the inner loop finds two results. Which should it take? Currently it will only take the last. \$\endgroup\$ – Reinderien Sep 16 '19 at 18:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.