Repository: brainglobe/brainreg (language: python)

Issue:
[BUG] Doublecheck additional channels don't overwrite each other
**Describe the bug**
[We have a _suspicion_ that if folders of additional channels contain files with the same name, the output file gets overwritten.](https://forum.image.sc/t/use-registered-atlases-from-brainreg-for-segmentation-in-imaris/86206/49?u=alessandrofelder)

**To Reproduce**
Please enter full details of how to reproduce this bug.

**Expected behaviour**
A clear and concise description of what you expected to happen.

**Log file**
Please attach the log file if relevant (e.g. `output/cellfinder_2019-12-17_13-29-46.log`)

**Screenshots**
If applicable, add screenshots to help explain your problem.

**Computer used (please complete the following information):**

- OS: [e.g. Linux, Windows, macOS] For macOS, please specify if you have an Intel or M1 CPU (M1 is likely if your mac is new).
- Version [e.g. 18.04]
- Hardware specs [e.g. 128 GB RAM, Xeon 4114 CPU].

**Additional context**
Add any other context about the problem here.



Discussion / hints:
This issue has been mentioned on **Image.sc Forum**. There might be relevant details there:

https://forum.image.sc/t/use-registered-atlases-from-brainreg-for-segmentation-in-imaris/86206/50

[Suspicion confirmed](https://forum.image.sc/t/use-registered-atlases-from-brainreg-for-segmentation-in-imaris/86206/54)
Ah, that was me over on image.sc. 
(https://github.com/brainglobe/brainreg/files/14471229/problem.solution.pdf)
A little more context, in case it helps.. 
My solution gives me a bit of anxiety, but it does the trick!

Fix patch:
diff --git a/brainreg/core/cli.py b/brainreg/core/cli.py
index 5752c67..cf4738d 100644
--- a/brainreg/core/cli.py
+++ b/brainreg/core/cli.py
@@ -197,16 +197,70 @@ def preprocessing_parser(parser):
 
 
 def prep_registration(args):
+    """
+    Prepares the file system for registration.
+
+    Ensures the brainreg directory exists and
+    stores paths for additional channels
+    that should be registered (if any were passed)
+    into a dictionary.
+
+    Dictionary keys for additional channel paths are
+    - the file/folder name passed (if unique for all additional channels)
+    - the file/folder name prefixed with the parent folder name (otherwise)
+
+    Throws an error if multiple additional channels have
+    the same name and the same parent folder, because the dictionary
+    key is not unique in this situation.
+
+    Parameters
+    ----------
+    args : argparse.Namespace
+        ArgParse object. Expected attributes here are "brainreg_directory"
+        and (optionally) "additional_images".
+
+    Returns
+    -------
+    tuple
+        A tuple containing the `args` object
+        and the `additional_images_to_downsample` dictionary.
+
+    """
     logging.debug("Making registration directory")
     ensure_directory_exists(args.brainreg_directory)
 
-    additional_images_downsample = {}
-    if args.additional_images:
-        for idx, images in enumerate(args.additional_images):
-            name = Path(images).name
-            additional_images_downsample[name] = images
+    additional_images_to_downsample = {}
 
-    return args, additional_images_downsample
+    if args.additional_images:
+        path_names = [
+            Path(path).name for path in args.additional_images
+        ]  # could be files or folders containing e.g. lots of 2D tiffs
+        has_duplicate_names = any(
+            path_names.count(name) > 1 for name in path_names
+        )
+        for additional_channel_paths in args.additional_images:
+            channel_name = Path(additional_channel_paths).name
+            if not has_duplicate_names:
+                additional_images_to_downsample[channel_name] = (
+                    additional_channel_paths
+                )
+            else:
+                channel_name = Path(additional_channel_paths).name
+                parent_folder = Path(additional_channel_paths).parent.name
+                channel_name = f"{parent_folder}_{channel_name}"
+                additional_images_to_downsample[channel_name] = (
+                    additional_channel_paths
+                )
+
+        assert len(args.additional_images) == len(
+            additional_images_to_downsample
+        ), (
+            "Something went wrong parsing additional channel names - "
+            + "please ensure additional channels have a unique "
+            + "combination of name and parent folder."
+        )
+
+    return args, additional_images_to_downsample
 
 
 def main():
diff --git a/pyproject.toml b/pyproject.toml
index 36d091c..b9ecfe4 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -46,6 +46,7 @@ dev = [
     "pre-commit",
     "pytest-cov",
     "pytest-qt",
+    "pytest-mock",
     "pytest",
     "setuptools_scm",
     "tox",


Test patch:
diff --git a/tests/tests/test_unit/test_cli.py b/tests/tests/test_unit/test_cli.py
new file mode 100644
index 0000000..f4dccbc
--- /dev/null
+++ b/tests/tests/test_unit/test_cli.py
@@ -0,0 +1,55 @@
+from pathlib import Path
+
+import pytest
+
+from brainreg.core.cli import prep_registration
+
+
+def test_prep_registration_different_names(mocker):
+    """Check that additional channel names are returned, when unique."""
+    args = mocker.Mock()
+    args.brainreg_directory = Path.home()  # This just needs to exist
+    args.additional_images = [
+        Path.home() / "additional_channel_0",
+        Path.home() / "additional_channel_1",
+    ]
+
+    _, additional_channel_outputs = prep_registration(args)
+    assert "additional_channel_0" in additional_channel_outputs
+    assert "additional_channel_1" in additional_channel_outputs
+
+
+def test_additional_channels_same_name_different_parent_name(mocker):
+    """
+    Check that parent folder name is part of keys returned
+    if additional channel names are not unique.
+    """
+    args = mocker.Mock()
+    args.brainreg_directory = Path.home()  # This just needs to exist
+    args.additional_images = [
+        Path.home() / "folder_0/duplicate_name",
+        Path.home() / "folder_1/duplicate_name",
+    ]
+
+    _, additional_channel_outputs = prep_registration(args)
+    assert "folder_1_duplicate_name" in additional_channel_outputs.keys()
+    assert "folder_1_duplicate_name" in additional_channel_outputs.keys()
+
+
+def test_prep_registration_same_name_same_parent_name(mocker):
+    """Check that error is thrown if both
+    parent and additional channel name are non-unique.
+    """
+    args = mocker.Mock()
+    args.brainreg_directory = Path.home()  # This just needs to exist
+    args.additional_images = [
+        str(Path.home() / "duplicate_name"),
+        str(Path.home() / "duplicate_name"),
+    ]
+
+    with pytest.raises(
+        AssertionError,
+        match=".*ensure additional channels have a unique "
+        + "combination of name and parent folder.*",
+    ):
+        prep_registration(args)