Sample Solution for COMP0233 Wk6, Ex 25-30

This repository contains a sample solution for the exercises #25 through #30 of COMP0233, week 6. These exercises are designed to guide you through the process of automating git bisect to identify when bugs were introduced to the codebase. Before beginning, you should remind yourself of the context of #24, which is the point at which we'll be working from. The key information is below:

• The function we are examining is run_averages, which takes 2D data as an input and writes the average of the rows to an output file.
• The bug is introduced in commit baef879.
• The bug is because the code is meant to average the rows of the data, but it is switched to averaging the columns of the data.

If you are starting this exercise having just done #24, you may need to exit git from a bisecting state:

git bisect reset
git switch main
git reset --hard HEAD

NOTE Charlene specifies that her code uses numpy version 1.17 (see #24). As such, before we begin we will create a new Python environment using conda (if you are not using conda to manage your Python environments, you will need to create a suitable environment using your chosen environment manager):

conda create -y -n sagittal_avg_env python numpy=1.17

which you can then activate using

conda activate sagittal_avg_env

Don't forget that, if you want to run your python scripts via the VSCode interface, you'll also need to change the Python interpreter to this new environment. Though since we're mainly using the command-line for git bisect here, this may not be necessary.

Jump to:

25 | 26 | 27 | 28 | 29 | 30 | Fixing the bug

Exercise #25

You can checkout this repository to commit 6090c4e to view the solution to just this part.

In this task we write a new file, test_sagittal_brain.py, and define input and expected numpy arrays to test Charlene's code.

The current input (brain_sample.csv) that Charlene has been using to test her code is not very useful because the column average of the data in this file is the same as the row average. As such, we should make sure that the input we define does not have this feature. Following the hint given in the issue, for some value N we can define an input array that looks like

[
    [0, 0, 0, 0, 0, 0, 0, ...],
    [1, 1, 1, 1, 1, 1, 1, ...],
    ...,
    [N-1, N-1, N-1, N-1, N-1, N-1, N-1, ...],
]

which has some important features:

• The average of the rows is the array [0, 1, ..., N-1].
• The average of the columns is the constant array [x, x, ...., x] where x = N*(N-1)/2.
• Importantly, the row average is no longer the same as the column average.

Exercise #26

You can checkout this repository to commit 63ecf75 to view the solution to just this part.

In this exercise we just need to use numpy to write the input array to the brain_sample.csv file, and then read the output from the brain_average.csv file. We can do this with the numpy.loadtxt and numpy.savetxt methods.

We edit our test_sagittal_brain.py file to do this, adding the following lines:

input_file = "brain_sample.csv"
output_file = "brain_average.csv"

# Save input array to the file we will give as an input to sagittal_brain.py
np.savetxt(
    input_file,  # write the input that the program will use
    input,  # array to write
    delimiter=",",  # data separator (CSV = comma separated values)
    fmt="%d",  # format of data (%d = write as integers)
)

# Load output array that run_averages will create,
# saving it to the output variable.
output = np.loadtxt(output_file, delimiter=",")

Exercise #27

You can checkout this repository to commit e1a9fc1 to view the solution to just this part.

We now need to have numpy test whether the output array that is read in is equal to the expected array that we defined earlier.

Following the hint, we can use the numpy.testing.assert_array_equal function to do this. Note that this function has an assert statement inside it, so it can double as something we could use in, say, a pytest test too.

Other possible ways of checking that two numpy arrays are equal (particularly if we're dealing with floats) is to use the numpy.allclose method.

We now update test_sagittal_brain.py again:

# Assert that output file contents match the expected values
np.testing.assert_array_equal(expected, output)

Exercise #28

You can checkout this repository to commit 49fe9f8 to view the solution to just this part.

The plan now is to turn our test_sagittal_brain.py file into something that runs Charlene's program in a subprocess, which you can think of as an isolated program that runs within a program.

We can call python scripts from the command line using python filename.py. The way that sagittal_brain.py expected to be called is:

python sagittal_brain.py <input_file>

Note that this way of calling Charlene's program is compatible with how she originally wrote the program (back on her first few commits) - this will be important later.

As such, we can create the test_call_command.py file and have it execute Charlene's program in a subprocess via:

import subprocess

if __name__ == "__main__":
    subprocess.run(
        [
            "python",
            "sagittal_brain.py",
            "brain_sample.csv",
        ]
    )

The if __name__ == "__main__": statement exists so that when we run test_call_command.py on the command-line, it will only run the code within this block. It is generally good practice to do this, as this stops Python from re-importing subprocess every time we call our test_call_command.py script on the command line. We can run our test_all_command.py file on the command line too;

python test_call_command.py

and this will then call Charlene's program with the input arguments we provided in the subprocess command.

Exercise #29

You can checkout this repository to commit 61d01b6 to view the solution to just this part.

We now want to move what we need from test_call_command.py into test_sagittal_brain.py, so our "test" is entirely contained in a single file. We are also told to use a good 20-by-20 input array (so we update our N to be 20), and to have Charlene's program run in between us writing the input to brain_sample.csv and loading the output from brain_average.csv.

As such, we move the subprocess.run call into test_sagittal_brain.py, to have it run between us writing input and reading output. We also put a suitable if __name__ == "__main__" block around our code, so that only the relevant parts run when we call it on the command line.

The final form of the test_sagittal_average.py file is then;

import subprocess
import numpy as np

# We want a 20-by-20 array
N = 20

# Our input and expected arrays don't change during runs
input = np.array([np.arange(N) for _ in range(N)]).T
expected = np.arange(N, dtype=float)

# Define which files to read / write from each time
input_file = "brain_sample.csv"
output_file = "brain_average.csv"

if __name__ == "__main__":
    # Save input array to the file we will give as an input to sagittal_brain.py
    np.savetxt(
        input_file,  # we want to write the input file that Charlene's program takes
        input,  # array to write
        delimiter=",",  # data separator (CSV = comma separated values)
        fmt="%d",  # format of data (%d = write as integers)
    )

    # Run Charlene's program before we then read the output.
    # We should also use our local variables containing the file names, rather than
    # the hardcoded values.
    subprocess.run(
        [
            "python",
            "sagittal_brain.py",
            input_file,
        ]
    )

    # Load output array that run_averages will create,
    # saving it to the output variable.
    output = np.loadtxt(output_file, delimiter=",")

    # Assert that output file contents match the expected values
    np.testing.assert_array_equal(expected, output)

Furthermore, we no longer need our test_call_command.py file, since we've rolled it into our test_sagittal_brain.py file. As such, we can delete it in this next commit we make too.

We can now run our test_sagittal_brain.py script in a bash terminal using

$ python test_sagittal_brain.py
AssertionError: 
Arrays are not equal

Mismatch: 100%
Max absolute difference: 9.5
Max relative difference: 1.
 x: array([ 0.,  1.,  2.,  3.,  4.,  5.,  6.,  7.,  8.,  9., 10., 11., 12.,
       13., 14., 15., 16., 17., 18., 19.])
 y: array([9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5,
       9.5, 9.5, 9.5, 9.5, 9.5, 9.5, 9.5])

As we expect, we get an AssertionError informing us that the expected array does not match what run_averages produced!

Exercise #30

You can checkout this repository to commit 038e379 to view the solution to just this part.

We are now going to use our test_sagittal_brain.py file to automate the git bisect process. Normally, when we run git bisect, we have to manually enter whether each commit we visit is good or bad. Now, we can just provide the starting good and bad commits, and then have git use our test_sagittal_brain.py file to determine if any subsequent commits it visits are good or bad.

We first need to determine the starting good and bad commits.

• Charlene's latest commit (d8bc3eb) is the starting "bad" commit. If we run our test_sagittal_brain.py script using the code at this commit, it throws the AssertionError we saw earlier, so we know it is not working.
• Charlene's second commit (9dc8a27) is the starting "good" commit. If we run our test_sagittal_brain.py script using the code at this commit, it doesn't throw any errors. We can even inspect the files that were generated to double check that the code did the right thing.

As such, we now have our starting "good" and "bad" commits. We can now run git bisect and have it use test_sagittal_brain.py to determine if the intermediate commits were good or bad. To do so, we follow these steps:

# Begin the bisection process
$ git bisect start
# Inform git of the starting bad commit
$ git bisect bad d8bc3eb
# Inform git of the starting good commit
$ git bisect good 9dc8a27
# Have git bisect run, but use test_sagittal_brain.py to
# determine if the intermediate commits are good or bad
$ git bisect run python test_sagittal_brain.py
baef87988e0e44ac753987206ae80b65fa3a906e is the first bad commit
commit baef87988e0e44ac753987206ae80b65fa3a906e
Author: Charlene Bultoc <c.bultoc@neurolab.ac.uk>
Date:   Sun Sep 29 05:35:31 2019 +0100

    Uses numpy to calculate the average

 sagittal_brain.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
bisect found first bad commit

And git has found us the first bad commit, baef87988e0e44ac753987206ae80b65fa3a906e, without us having to manually tell it if the intermediate commits it wanted to investigate were good or bad!

NOTE: An important thing to notice here is that, if we did not create a new Python environment to replicate Charlene's, our script would actually error when bisect was running. This is because numpy version 2+ has since been released, which includes breaking changes from version 1 (which Charlene is using)! As such, it is very important to replicate the environment in which you are trying to debug.

Extra: Turning our script into a test

You can checkout this repository to commit afdb453 to view the solution to just this part.

Having done this debugging, it is probably a good idea for us to both

• fix the bug,
• and add a test so that the bug doesn't come back!

Luckily for us, our test_sagittal_brain.py file provides a great starting point for such a test. But there are a few things we should address:

• We should test the run_averages function, rather than running the sagittal_brain.py script. This is because we have identified that the bug was introduced in commit baef879, and the only lines changed in this commit were inside the run_averages file. It is also preferable to avoid calling subprocess in your tests, unless there is really no other way of running your code (as it means that if your command-line syntax changes, your test doesn't need to change with it). As such, rather than using subprocess, we will instead use from sagittal_brain import run_averages and then call run_averages directly.
• We should not write to the brain_samples.csv and brain_average.csv files. These files are tracked by the repository, and we don't want our tests to overwrite them every time we run them! As such, we will use pytest's tmp_path fixture that creates a temporary test directory, and save our input (and output) files to this temporary directory. That way, we won't interfere with the files in the repository themselves.
• We should also try and refactor out some of the variables that we are using in this test. Things like N, the names of the input_file and output_file, come to mind here.

After some suitable edits, we will obtain a pytest-ready test file in test_sagittal_brain.py;

from pathlib import Path

import numpy as np

from sagittal_brain import run_averages


def test_sagittal_brain_averages_rows(
    tmp_path: Path, N: int = 20, input_file="input.csv", output_file="output.csv"
) -> None:
    """Assert that run_averages correctly loads a file and computes the row-wise
    average of the data contained in the file.

    Write input and output files to a temporary directory, so that we do not
    interfere with nor change other files already committed to the repository.
    """
    # Our input and expected arrays don't change during runs
    input = np.array([np.arange(N) for _ in range(N)]).T
    expected = np.arange(N, dtype=float)

    # Ensure that we are writing files to the temporary directory
    input_file = tmp_path / input_file
    output_file = tmp_path / output_file

    # Save input array to the file we will give as an input to sagittal_brain.py
    np.savetxt(
        input_file,  # we want to write the input file that Charlene's program takes
        input,  # array to write
        delimiter=",",  # data separator (CSV = comma separated values)
        fmt="%d",  # format of data (%d = write as integers)
    )

    # Run run_averages, passing in the temporary file paths that we created
    run_averages(file_input=input_file, file_output=output_file)

    # Load output array that run_averages will create,
    # saving it to the output variable.
    output = np.loadtxt(output_file, delimiter=",")

    # Assert that output file contents match the expected values
    np.testing.assert_array_equal(expected, output)

Or course, running

pytest test_sagittal_brain.py

still tells us that the code errors - because we didn't actually fix the bug! We can do this by fixing line 19 of sagittal_brain.py, it should read

averages = planes.mean(axis=1)[np.newaxis, :]

instead of

averages = planes.mean(axis=0)[np.newaxis, :]

This is because numpy.mean treats axis=0 as "taking the average across the rows (IE, of the values in each column)" and axis=1 as "taking the average across the columns (IE, of the values in each row)".

And we now have some working code again, and a test to ensure that it never comes back! At this point we would probably open a pull request, with our bug fix and new test, to Charlene's repository for her to review.