Fix: data split issue

[jeongHwarr]

Description

• Provide a summary of the modification as well as the issue that has been resolved. List any dependencies that this modification necessitates.

• Fixes When test_split is applied to a custom dataset, test data contamination occurs #403

Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist

• My code follows the pre-commit style and check guidelines of this project.
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing tests pass locally with my changes

[ashwinvaidya17]

What's wrong with setting the seed to a value other than 0? If we make this change, we will have to ensure that lines like

if config.project.seed != 0:
        seed_everything(config.project.seed)

are changed in other files - train.py, etc

[jeongHwarr]

@ashwinvaidya17

If seed is non-zero, there is no problem.
However, if the seed is 0, the test data is also included in the training data. So the performance is abnormally high.

Does seed 0 means that the seed is not fixed in this project?
If so, the code seems to need a lot of modification.

It seems that samples are shared rather than made the same seed.
https://github.com/openvinotoolkit/anomalib/blob/83645772754a1b7d909930b9b49726ed460696d8/anomalib/data/folder.py#L163-L166

Now, the PR code solves the data contamination problem, but each traininig data and test data are created identically for the seed 0.

[ashwinvaidya17]

@jeongHwarr not sure if I quite understand the contamination problem. Currently we treat seed 0 as having no seed. However, even with seed 0 I get no overlap between train split and the test split. Here are the sample lines of code I am using for testing this. This does not raise Assertion Error for multiple runs.

from anomalib.data.folder import make_dataset
from pathlib import Path

dataset = make_dataset(normal_dir=Path("../datasets/bottle/good/"), abnormal_dir=Path('../datasets/bottle/broken_large'), split_ratio=0.33, seed=0)

train_split = dataset[dataset.split == "train"]
test_split = dataset[dataset.split == "test"]
assert train_split["image_path"].values.all() != test_split["image_path"].values.all()

Here is another sample using Folder DataModule

folder_dataset = Folder(root=Path("../datasets/bottle"),normal_dir="good", abnormal_dir='broken_large', split_ratio=0.1, seed=0, image_size=256)
folder_dataset.setup()
assert folder_dataset.test_data.samples["image_path"].values.all() != folder_dataset.train_data.samples["image_path"].values.all()

[jeongHwarr]

@ashwinvaidya17

There is no overlap in your test code because you called make_dataset only once.

However, in the code of this project, make_dataset is called multiple times. At this time, if the seed is not fixed, the sample will not come out the same.
https://github.com/openvinotoolkit/anomalib/blob/83645772754a1b7d909930b9b49726ed460696d8/anomalib/data/folder.py#L163-L166
To reproduce this situation, set normal_dir to None in yaml, let the above part be called, and then compare the training data and test data. Then you can find the overlap there.

make_dataset is called multiple times through the code below.
https://github.com/openvinotoolkit/anomalib/blob/83645772754a1b7d909930b9b49726ed460696d8/anomalib/data/folder.py#L476-L523

https://github.com/openvinotoolkit/anomalib/blob/83645772754a1b7d909930b9b49726ed460696d8/anomalib/data/folder.py#L246-L256

To solve this problem, make_datset is called only once, and the below part in make_dataset must be processed in another part not in the make_datset.
https://github.com/openvinotoolkit/anomalib/blob/83645772754a1b7d909930b9b49726ed460696d8/anomalib/data/folder.py#L172-L175

[ashwinvaidya17]

@jeongHwarr I see what you mean. Thanks for spotting this. I'll look into the issue. But does it make sense to keep seed 0 as no seed or should we refactor the code to make seed: null as no seed?

[ashwinvaidya17]

@jeongHwarr Thanks again for spotting this issue and creating a PR for addressing it. Is it possible for you to make a few changes to this PR (https://github.com/openvinotoolkit/anomalib/pull/437/files) so that we can merge this and you can be a contributor to this repo 😀. Other option is to always merge this and I can create a new PR to address this.

[jeongHwarr]

@ashwinvaidya17

I think that None or null is more clear than seed 0.
Until I saw the code of this repo, I thought that it was fixed as seed 0.

I've reviewed your PR and nothing seems to change!
Thank you 😀