4
\$\begingroup\$

I would like to refactor a large Python method I wrote to have better practices.

I wrote a method that parses a design file from the FSL neuroimaging library. Design files are text files with settings for the neuroimaging software. I initially wrote the code to do all the processing in a single loop though the file.

I'm looking for all manner of suggestions, but mostly tips of design and best practices. I'd love input on the project as a whole, but I know that might be out of scope for this site.

The code structure:

  1. Define variables based on type of design file
  2. Parse design file based on type of design file
  3. Return the now populated variables setup in 1

You can also view the code on GitHub along with the larger project.

def parse_design_file(self,fsf_lines, type):
    """
        Parses design file information and return information in parsed variables that can be used by the csv methods
    """
    analysis_name=''
    output_path=''
    zvalue=''
    pvalue=''

    if type == self.FIRST_TYPE or self.PRE_TYPE:
        in_file=''
    if type == self.FIRST_TYPE:
        ev_convolves=dict()
        ev_paths=dict()
        ev_deriv=dict()
        ev_temp=dict()
    if type == self.ME_TYPE or type == self.FE_TYPE:
        feat_paths=dict()
        count=''
    if type == self.FIRST_TYPE or type == self.FE_TYPE:
        ev_names=dict()
        evg_lines=list()
        cope_names=dict()
        cope_def_lines=list()
    if type == self.PRE_TYPE:
        tr=''
        total_volumes=''
        brain_thresh=''
        motion_correction=''
        smoothing=''
        deleted=''
    if type == self.FE_TYPE:
        first_example_dir=''
    if type == self.ME_TYPE:
        FE_example_dir=''

    for line in fsf_lines:
        #regex matching
        #all
        output_match=re.search("set fmri\(outputdir\)",line)
        feat_file_match=re.search("feat_files\(\d+\)",line)
        total_vols_match=re.search("fmri\(npts\)", line)
        z_match=re.search("set fmri\(z_thresh\)",line)
        p_match=re.search("set fmri\(prob_thresh\)",line)

        if output_match:
            output_path=self.get_fsf_value(line,output_match.end())
            #TODO hardcoded stripping here, make flexible
            if type == self.ME_TYPE:
                run=re.search("ME",line)
            elif type == self.FIRST_TYPE or type == self.PRE_TYPE:
                run=re.search("r\d\/",line)
            elif type == self.FE_TYPE:
                run=re.search("FE\d*",line)
            if run:
                analysis_name=self.get_fsf_value(line,run.end())
            if type == self.ME_TYPE:
                analysis_name=self.strip_cope(analysis_name)

        if total_vols_match:
            value=self.get_fsf_value(line,total_vols_match.end())
            if type == self.ME_TYPE or type == self.FE_TYPE:
                count=value
        if z_match:
            value=self.get_fsf_value(line,z_match.end())
            zvalue=value

        if p_match:
            value=self.get_fsf_value(line,p_match.end())
            pvalue=value

        if feat_file_match:
            if type == self.FIRST_TYPE or type == self.PRE_TYPE:
                value=self.get_fsf_value(line,feat_file_match.end())
                in_file=self.strip_root(value)
                preproc_match=re.search("preproc.*feat",value)
                #TODO inconsistent methodology here
                if preproc_match:
                    self.preproc=value[preproc_match.start():preproc_match.end()]
                    print self.preproc
            elif type == self.ME_TYPE or type == self.FE_TYPE:
                value=self.get_fsf_value(line,feat_file_match.end())
                index=self.get_fsf_indice(feat_file_match.group())
                stripped=self.strip_fanal(line)
                feat_paths[index]=stripped
                if (type == self.ME_TYPE and not FE_example_dir) or (type == self.FE_TYPE and not first_example_dir):
                    set_match=re.search("set feat_files\(\d+\) \"",line)
                    no_cope=line[set_match.end():len(line)]
                    no_cope=no_cope.strip('\n')
                    no_cope=no_cope.strip('\"')
                    no_cope=self.strip_cope(no_cope)
                    if type == self.ME_TYPE:
                        FE_example_dir=no_cope
                    else:
                        first_example_dir=no_cope

        if type == self.PRE_TYPE:
            tr_match=re.search("fmri\(tr\)", line)
            mc_match=re.search("set fmri\(mc\)",line)
            smooth_match=re.search("set fmri\(smooth\)",line)
            total_vols_match=re.search("fmri\(npts\)", line)
            removed_volumes=re.search("fmri\(ndelete\)", line)
            thresh_match=re.search("set fmri\(brain_thresh\)",line)

            if tr_match:
                tr=self.get_fsf_value(line,tr_match.end())
            if mc_match:
                value=self.get_fsf_value(line,mc_match.end())
                if value == "1":
                    value = "Y"
                else:
                    value = "N"
                motion_correction=value
            if smooth_match:
                smoothing=self.get_fsf_value(line,smooth_match.end())
            if removed_volumes:
                deleted=self.get_fsf_value(line,removed_volumes.end())
            if total_vols_match:
                total_volumes=self.get_fsf_value(line,total_vols_match.end())
            if thresh_match:
                brain_thresh=self.get_fsf_value(line,thresh_match.end())


        if type == self.FIRST_TYPE:
            ev_conv_match=re.search("fmri\(convolve\d+\)", line)
            ev_path_match=re.search("fmri\(custom\d+\)", line)
            ev_deriv_match=re.search("fmri\(deriv_yn\d+\)", line)
            ev_temps_match=re.search("fmri\(tempfilt_yn\d+\)", line)

            if ev_conv_match:
                conv=self.get_fsf_value(line,ev_conv_match.end())
                index=self.get_fsf_indice(ev_conv_match.group())
                conv_text={
                    "0" : "none",
                    "1" : "Gaussian",
                    "2" : "Gamma",
                    "3" : "Double-Gamma HRF",
                    "4" : "Gamma basis functions",
                    "5" : "Sine basis functions",
                    "6" : "FIR basis functions",
                    }
                ev_convolves[index]=conv_text[conv]

            if ev_deriv_match:
                value=self.get_fsf_value(line,ev_deriv_match.end())
                index=self.get_fsf_indice(ev_deriv_match.group())
                if value == "1":
                    value = "Y"
                else:
                    value = "N"
                ev_deriv[index]=value
            if ev_temps_match:
                value=self.get_fsf_value(line,ev_temps_match.end())
                index=self.get_fsf_indice(ev_temps_match.group())
                if value == "1":
                    value = "Y"
                else:
                    value = "N"
                ev_temp[index]=value
            if ev_path_match:
                value=self.get_fsf_value(line,ev_path_match.end())
                index=self.get_fsf_indice(ev_path_match.group())
                ev_paths[index]=self.strip_root(value)

        if type == self.FE_TYPE:
            evg_match=re.search("fmri\(evg\d+\.\d+\)", line)
            if evg_match:
                evg_lines.append(line)
        if type == self.FE_TYPE or type == self.FIRST_TYPE:
            ev_name_match=re.search("fmri\(evtitle\d+\)", line)
            cope_name_match=re.search("fmri\(conname_real\.\d+\)", line)
            cope_def_match=re.search("fmri\(con_real\d+\.\d+\)", line)
            if cope_name_match:
                name=self.get_fsf_value(line,cope_name_match.end())
                index=self.get_fsf_indice(cope_name_match.group())
                cope_names[index]=name
            if cope_def_match:
                cope_def_lines.append(line)
            if ev_name_match:
                name=self.get_fsf_value(line,ev_name_match.end())
                index=self.get_fsf_indice(ev_name_match.group())
                ev_names[index]=name

    if type == self.FIRST_TYPE or type == self.FE_TYPE:
        design_matrix=[['0' for col in range(len(ev_names)+2)] for row in range(len(cope_names)+1)]
        if 'ev_temp' in locals():
            real_copes=list()
            index_cope=1
            real_copes.append(str(index_cope))
            for i in range(1,len(ev_temp)+1):
                ind=str(i)
                if ev_temp[ind] == 'Y':
                    index_cope += 2
                else:
                    index_cope += 1
                real_copes.append(str(index_cope))
            real_copes.pop()
            design_matrix=self.fill_matrix(cope_def_lines,design_matrix,type,1,real_copes)
        else:
            design_matrix=self.fill_matrix(cope_def_lines,design_matrix,type,1)

        for i in range(1,len(cope_names)+1):
            ind=str(i)
            design_matrix[i][0]=ind
        for i in range(1,len(cope_names)+1):
            ind=str(i)
            design_matrix[i][1]=cope_names[ind]
        for i in range(2,len(ev_names)+2):
            ind=str(i-1)
            design_matrix[0][i]=ev_names[ind]
        design_matrix[0][0]='Cope #'
        design_matrix[0][1]='Cope Name'
    if type == self.PRE_TYPE:
        return analysis_name,output_path,tr,total_volumes,deleted,in_file,motion_correction,brain_thresh,smoothing
    elif type == self.FIRST_TYPE:
        return analysis_name,output_path,in_file,design_matrix,ev_names,ev_paths,ev_convolves,ev_deriv,ev_temp,cope_names
    elif type == self.ME_TYPE:
        return analysis_name,output_path,pvalue,zvalue,feat_paths,count, FE_example_dir
    elif type == self.FE_TYPE:
        regressor_matrix=[['0' for col in range(int(count)+1)] for row in range(len(ev_names)+1)]
        self.fill_matrix(evg_lines,regressor_matrix,5, 0)
        for i in range(1,len(ev_names)+1):
            ind=str(i)
            regressor_matrix[i][0]=ev_names[ind]
        for i in range(1,int(count)+1):
            ind=str(i)
            regressor_matrix[0][i]=ind
        return analysis_name,output_path,feat_paths,count,design_matrix,regressor_matrix,ev_names,cope_names,first_example_dir
\$\endgroup\$
3
\$\begingroup\$

Your function acts widely different depending on the value of the type parameter. In fact, it basically acts like different functions depending on that parameter. It returns completely different sets of values depending on the value of type. The function would be better off split into several functions, one for each type. Then each function would be simpler, and easier to follow.

I assume that you have done it this way to try and share code between the different filetypes. This is the wrong way to do it. You should share code by calling common functions or using common base classes. You should not share code by having functions exhibit widely different behavior.

For parsing, I'd suggest you actually parse the file, not just run some regular expressions over it. From what I gather your file basically consistents of lines of the form:

set something somevalue

I'd write a parse() function that converts the file into a dictionary. So this:

# Threshold IC maps
set fmri(thresh_yn) 1
set fmri(mmthresh) 0.5
set fmri(ostats) 0

Would become

{'fmri(thesh_yn)' : 1, 'fmri(mmthres)' : 0.5, 'fmri(ostats)' : 0}

Then the function for a particular type can just look up entries in the dictionary.

In the end, you return a long tuple. Why? Long tuples are difficult to work with. Perhaps you should really be storing them on an object with many attributes?

\$\endgroup\$
  • \$\begingroup\$ This is a dramatic improvement, thank you very much Winston. When you say: "Perhaps you should really be storing them on an object with many attributes?" Do you mean create a separate class and populate the attributes? \$\endgroup\$ – Mike May 23 '12 at 6:43

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.