Added brute force method to preserved site k-fold split

[rsethi21]

No description provided.

[jamesdolezal]

Hi Rohan - please see my comments here. There are a couple of phases we'll take for this review - python basics, overall algorithm design, and then optimizations. I've focused mostly on python basics here. With your next update, I hope to move to overall algorithm design.

I would recommend reading about list (and dictionary) comprehension - it's Python magic that both speeds up code and significantly improves readability. Many of the loops you have could probably be replaced with some form of list/dictionary comprehension.

We can talk about the overall algorithm next, but I wanted to work on improving the readability a bit, as it becomes MUCH easier to optimize an algorithm if the code is already pretty lean.

[jamesdolezal]

See above regarding test functions in the main module.

Additionally, you shouldn't leave commented out code in a committed file - it should either be deleted, or put into a separate function.

[jamesdolezal]

Test functions shouldn't be in the main module - rather, they should be in separate files used for unit testing. In this case, the site splits have unit tests under slideflow/test/dataset_test.py:

https://github.com/jamesdolezal/slideflow/blob/master/slideflow/test/dataset_test.py

You could add an additional unit test in the TestSplits class, which tests your new method. The TestSplits.setUpClass() method prepares some test data, which is then used by each of the unit tests.

For this exercise, I'll have you hold off on implementing unit tests, as I recently made changes to that module that haven't yet been integrated. We'll circle back to the unit tests later.

[jamesdolezal]

Error handling should use the try...except Python functionality rather than if...then statements, as it allows for large applications to properly report and handle errors.

In this case, is the number of crossfolds is greater than the number of unique sites, we would want to raise an error, since it's not possible to perform site-preserved splitting. The type of error that should be raised will depend on the surrounding application and should make sense. In this case, the best error to raise would probably be slideflow.errors.DatasetSplitError. So you would change that block to:

if crossfolds > len(unique_sites):
    raise sf.errors.DatasetSplitError(
        "Insufficient number of sites ({}) for number of crossfolds ({})".format(
            len(unique_sites),
            crossfolds))

That also enables you to get rid of the following else statement and de-indent the next code block by a level.

[jamesdolezal]

Instead of setting variables to list() and dict(), python convention is to use [] and {}

[jamesdolezal]

the int() is redundant here, as len() always returns an integer

[jamesdolezal]

Rather than looping and counting lengths with the item_length counter, the clean pythonic way of doing this would be:

sum([len(i) for i in item])

[jamesdolezal]

What is the role of list_of_items_in_a_combo here? Is it not the same as item after the loop on lines 52-54?

[rsethi21]

Since the item is a list of lists and I need the list of sites from all of those lists; I changed it to a list comprehension instead.

sites_in_a_possible_crossfold = [site for site in possible_crossfold] where

• possible_crossfold was item
• where sites_in_a_possible_crossfold was list_of_items_in_a_combo
  @jamesdolezal does that work better

[jamesdolezal]

This line:

sites_in_a_possible_crossfold = [site for site in possible_crossfold]

is equivalent to:

sites_in_a_possible_crossfold = possible_crossfold

If this is what you are intending, you should consider removing the sites_in_a_possible_crossfold entirely and just use possible_crossfold. Does that make sense, or am I missing something?

[jamesdolezal]

Instead of float(1), do 1.

[jamesdolezal]

sum2 is not a descriptive variable name

[jamesdolezal]

What is the purpose of min here? What are you actually trying to check for with the if value < min statement?

Additionally, min() is a python standard function (finds the minimum of a list, similar to max(), so you should name your variable something else.

[rsethi21]

so i am using that as an initial value to start comparing the errors in the rest of the dictionary; the value<min will check to see if the values are less than that. If so, it resets min to be value and continues looking for the smallest error.

[jamesdolezal]

I see. In that case, you should remove the min variable (having a manually set large value is clunky). Instead, you can just sort the dictionary and find the lowest value. So instead of:

min = 100000000000000000000000000000000000000000000
best_combo = None
for key, value in per_combo_errors.items():
    if value < min:
        best_combo = key
        min = value
    else:
        pass

do

best_combo = min(per_combo_errors, key=per_combo_errors.get)

[jamesdolezal]

In a083554, I add a unit test that ensures the generated splits are valid (all patients are used, no site is present in multiple cross-folds). The test generated data was copied from your original submission.

Running the test only requires:

python3 crossfolds_test.py

[jamesdolezal]

In 57ac792, I made formatting changes with minor refactoring only to improve readability - the underlying algorithm is the same. Code that is easier to read is also easier to refactor and optimize. The kinds of changes I made include:

• Added typing and docstring to the function declaration, to make it clear what the input arguments are and what the function does.
• Broke long code blocks into smaller discrete sections, with accompanying comments to explain what each section does.
• More succinct and easily interpretable variable names
• Line length of 80
• List comprehension to reduce the number of nested loops

With the unit testing now available, we can ensure that the splits that are being generated are valid. Running the unit test both pre- and post-refactor raises an error, indicating an issue with the algorithm.

For this next step, I'll have you familiarize yourself with the modified code, and track down the cause of the failed unit test. Once the unit test passes indicating that the algorithm is complete, we will move on to optimization.