Updated coreset subsampling method to improve accuracy #73
Conversation
| @@ -14,7 +14,7 @@ dataset: | |||
| use_random_tiling: False | |||
| random_tile_count: 16 | |||
| image_size: 224 | |||
| train_batch_size: 32 | |||
| train_batch_size: 1 | |||
There was a problem hiding this comment.
Is this by choice or an artefact of debugging?
There was a problem hiding this comment.
Good catch. It was an artifact. Fixed it.
anomalib/models/patchcore/model.py
Outdated
| embedding: (sample_count, d) tensor of patches. | ||
| sample_count: Number of patches to select. | ||
| eps: Parameter for spare projection aggresion. | ||
| force_cpu: Force cpu, useful in case of GPU OOM. |
There was a problem hiding this comment.
These seem to be missing the types.
| @@ -14,7 +14,7 @@ dataset: | |||
| use_random_tiling: False | |||
| random_tile_count: 16 | |||
| image_size: 224 | |||
| train_batch_size: 32 | |||
| train_batch_size: 1 | |||
There was a problem hiding this comment.
Why train_batch_size is set to 1?
There was a problem hiding this comment.
Good catch. It was an artifact. Fixed it.
anomalib/models/patchcore/model.py
Outdated
| transformer = random_projection.SparseRandomProjection(eps=eps) | ||
| sample_set = torch.tensor(transformer.fit_transform(sample_set)) # pylint: disable=not-callable | ||
| except ValueError: | ||
| print(" Error: could not project vectors. Please increase `eps` value.") |
There was a problem hiding this comment.
Same as above. Add logger todo
anomalib/models/patchcore/model.py
Outdated
| print("Fitting random projections...") | ||
| try: | ||
| transformer = random_projection.SparseRandomProjection(eps=eps) | ||
| sample_set = torch.tensor(transformer.fit_transform(sample_set)) # pylint: disable=not-callable |
There was a problem hiding this comment.
We could maybe move to GPU like the following:
sample_set = torch.tensor(transformer.fit_transform(sample_set.cpu())).to(sample_set.device)
This would automate the assign to the device.
anomalib/models/patchcore/model.py
Outdated
| if torch.cuda.is_available() and not force_cpu: | ||
| last_item = last_item.to("cuda") | ||
| sample_set = sample_set.to("cuda") | ||
| min_distances = min_distances.to("cuda") |
There was a problem hiding this comment.
When the device is automatically assigned as suggeste above in Line 320, we wouldn't need to move to cuda.
anomalib/models/patchcore/model.py
Outdated
|
|
||
| last_item = sample_set[select_idx : select_idx + 1] | ||
| min_distances[select_idx] = 0 | ||
| coreset_idx.append(select_idx.to("cpu")) # type: ignore[attr-defined] |
There was a problem hiding this comment.
why is this moved to cpu?
There was a problem hiding this comment.
It was due to the way select_idx was initialized. Updated this in the latest commit.
|
Thanks for the review @samet-akcay @ashwinvaidya17 . I have addressed them in the recent commit. |
There was a problem hiding this comment.
@blakshma Thanks a lot for this PR. Great to finally see the Patchcore implementation produce results that are in line with the paper. Also nice to get rid of the helper classes, the implementation is much cleaner this way. Just two minor comments.
anomalib/models/patchcore/model.py
Outdated
| """Returns n subsampled coreset idx for given sample_set. | ||
|
|
||
| Args: | ||
| embedding (Tensor): (sample_count, d) tensor of patches. |
There was a problem hiding this comment.
This naming differs from the method signature
anomalib/models/patchcore/model.py
Outdated
|
|
||
| return batch | ||
|
|
||
| def get_coreset_idx( |
There was a problem hiding this comment.
I feel that maybe this method should be part of the PatchcoreModel class. In the other implementations we reserve the lightning class for the learning and inference logic, and define any model-specific operations in the Torch class. Maybe it could be called create_coreset or subsample_coreset and placed in PatchcoreModel. It could also assign the embeddings attribute from within the method so that it doesn't need to return anything. This would keep the lightning class a bit cleaner.
There was a problem hiding this comment.
That's a great idea. I have made that change.
This reverts commit 3613a6e.
Fixes #74 and #6