3
\$\begingroup\$

I have a file that at some point contains a line of the following form:

OutputFilename output.root

I want to write a Python function that replaces the text after "OutputFilename" and saves to a new file.

I have made the following attempt, which works, but I'm sure there must be a more efficient, Pythonic way to do this.

def set_cuts_configuration_output_filename_configuration(
    cuts_filename                 = "cuts.txt",
    output_filename_configuration = "output_new.root",
    output_cuts_filename          = "cuts_new.txt"
    ):
    # Access the cuts configuration.
    with open (cuts_filename, "r") as input_file:
        cuts_file = input_file.readlines()
    # Find the output filename line.
    modified_file = []
    for line in cuts_file:
        if "OutputFilename" in line:
            modified_file.append(line.split(" ")[0] + " " + output_filename_configuration + "\n")
        else:
            modified_file.append(line)
    # Save the new file content.
    output_cuts_file = open(output_cuts_filename, "w")
    for line in modified_file:
        output_cuts_file.write(line)
    output_cuts_file.close()
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Instead of opening and closing the files separately, use with open(file,mode) as f \$\endgroup\$
    – hjpotter92
    Nov 23, 2015 at 13:11
  • \$\begingroup\$ @hjpotter92 That won't work if you want the output to go to a separate file. \$\endgroup\$ Nov 24, 2015 at 2:59

3 Answers 3

4
\$\begingroup\$

Here are some comments to your solution:

  • Try avoiding reading the entire files into memory – Using readlines() means you read the entire file into memory, which can be eat a lot of memory. Usually it is better to read and process the file line by line
  • Use with open(...) for pythonic file reading/writing – This can be used both for reading and writing files, and can also be combined into a double with in your case to read and write at the same time
  • Make functions a little more generic – To have a function to change just one explicitly named config value seems a little strange. Two natural extensions: 1) Allow for an arbitrary config value to be changed, 2) Allow for multiple config values to be changed in one go.
  • Note that "OutputFilename" in line matches in the entire line – If you write "OutputFilename" at end of line it matches, or at start, or as part of another word. This could lead to a lot of failing changes.
  • Set the new config line directly – If you have a positive hit on the line you want to change, you know how the line is supposed to look, so no need to split, append and change it. Set it directly to the correct value.
  • Add some blank lines in your script to make it easier to read – A lot of people are afraid of having blank lines in script, and that is said. It can make the script a lot easier to read.

Refactored code

Here is two functions (within a program) to illustrate a better approach:

def change_configuration_option(
    option,
    new_value,
    sourcefile = "cuts.txt",
    destinationfile = "cuts_new.txt"):
    """Change option_name in sourcefile to new_value in destinationfile."""

    with open(sourcefile) as source, open(destinationfile, "w") as destination:
        for line in source:

            if option in line: # Buggy match on line...
                destination.write('{} {}\n'.format(option, new_value))
            else:
                destination.write(line)


def change_configuration_options(
    change_options,
    sourcefile="cuts.txt",
    destinationfile = "cuts_multiple.txt"):
    """From sourcefile to destinationfile do change_options."""

    with open(sourcefile) as source, open(destinationfile, "w") as destination:
        for line in source:

            # For every line, match against new_options
            for option in change_options:
                if line.startswith(option + ' '):
                    destination.write('{} {}\n'.format(option, change_options[option]))
                    break

            else:
                destination.write(line)


def main():
    """Replace output.root with new_output.root in config file."""

    change_configuration_option("OutputFilename", "new_output_file.txt")

    change_configuration_options(
        { "OutputFilename" : "new_output_file.txt",
          "YetAnother" : "bites the dust",
          "AnotherConfig" : "And another one",
         })

if __name__ == '__main__':
    main()

The change_configuration_options() takes a dictionary of new configuration option changes, and it uses a little trick within the inner loop to check write non-matching options to the output file. That trick is the for ... else loop. If the for loop completes normally, the else will execute, but if a matching option is found and it break's out of the loop then the else: will not be executed.

Also note that in the multiple variant, I've changed the matching on the line to be line.startswith(option + " "), which limits the option to be at the start of the line, and a complete word followed by a space. This avoids false hits, as are present in your original code and in my change_configuration_option() for changing the single option. (Left as proof of concept, see tests below)

Test run

I tested this using the following cuts.txt file:

MyOutputFilename Not_this_file
OutputFilename this_file_should_change
# Next line should have value "no change"
OutputFilenameAndSome no change
# Multiple
AnotherConfig AnotherValue
YetAnother config value with multiple spaces
# Flawed
FinalFlaw What if the value has OutputFilename as part of text?

And after running, I got the file cuts_new.txt:

OutputFilename new_output_file.txt
OutputFilename new_output_file.txt
# Next line should have value "no change"
OutputFilename new_output_file.txt
# Multiple
AnotherConfig AnotherValue
YetAnother config value with multiple spaces
# Flawed
OutputFilename new_output_file.txt

Notice the duplicated line near start, due to falsely changing the OutputFilenameAndSome option, and the changed bottom line when comparing to original file.

And finally the file cuts_multiple.txt:

MyOutputFilename Not_this_file
OutputFilename new_output_file.txt
# Next line should have value "no change"
OutputFilenameAndSome no change
# Multiple
AnotherConfig And another one
YetAnother bites the dust
# Flawed
FinalFlaw What if the value has OutputFilename as part of text?

This file has only the intended changes. The latter option should be a better alternative for changing lines in a file, and should be quite efficient as well. A further optimisation, could be to remove already changed option from the change_options dict after doing that line, but then again that could be unneccessary micro optimisation.

The provided solutions reads and writes the file in one go, and only keeps one line of the file in memory (except internal cacheing). In most operating systems, this should be the better option for handling an arbitrarily sized file. (Earlier Windows version could loose some efficiency due to the almost simultaneously read and write to two different places/files at the same time.)

\$\endgroup\$
1
\$\begingroup\$

Replace the if stmt with,

if "OutputFilename" in line:
    modified_file.append(re.sub(r'\bOutputFilename\b.*'  , 'OutputFilename' + ' ' + output_filename_configuration , line))
\$\endgroup\$
0
\$\begingroup\$

That's one massive function name. It indicates that you're either being too specific or the function is. Why not edit_cuts_config? You can explain it clearer in a docstring afterwards, but the function name should be shorter. I try to avoid more than three words in a name.

Your parameters (aside from also being long) are laid out oddly, like they're in a dictionary. I didn't even realise they were parameters at first. I'd rewrite the start this way:

def edit_cuts_config(input_filename="cuts.txt",
                     output_config="output_new.root",
                     output_filename="cuts_new.txt"):
    """Reads a cuts file and writes an updated one.

    input_filename is the read file
    output_filename is the file written
    output_config is what replaces "OutputFilename" in the source file.
    """

Reading all the lines into memory is generally less performant. You could do it a lot easier if you read and write at the same time. Luckily with allows you to open multiple files at once:

with (open(cuts_filename, "r") as input_file,
      open(output_cuts_filename, "w") as output_file):

Now you can read and write at the same time. So you can just iterate over the input_file and write each line to the output_file. This is much faster and more direct:

with (open(cuts_filename, "r") as input_file,
      open(output_cuts_filename, "w") as output_file):

    for line in input_file:
        output_file.write(line)

Of course you want to add your if condition in there. But there's an easier way to do it than you had:

    if "OutputFilename" in line:
        output_file.write("OutputFilename " + output_filename_configuration + "\n")

Since it's always based on "OutputFilename" being in the line then there's no need to split and use the line. If anything from the line needed to be preserved then your original script didn't do it either.

Putting that all together you have a much shorter, cleaner and faster function:

def edit_cuts_config(input_filename="cuts.txt",
                     output_config="output_new.root",
                     output_filename="cuts_new.txt"):
    """Reads a cuts file and writes an updated one.

    input_filename is the read file
    output_filename is the file written
    output_config is what replaces "OutputFilename" in the source file.
    """

    with (open(cuts_filename, "r") as input_file,
          open(output_cuts_filename, "w") as output_file):

        for line in input_file:
            if "OutputFilename" in line:
                output_file.write("OutputFilename " 
                                  + output_filename_configuration + "\n")
            else:
                output_file.write(line)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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