PIMO

[jpcbertoldo]

📝 Description

Replace #1557, which replaces the PRs from https://gist.github.com/jpcbertoldo/12553b7eaa97cfbf3e55bfd7d1cafe88 .

Implements refactors from https://github.com/jpcbertoldo/anomalib/blob/metrics/refactors/src/anomalib/utils/metrics/perimg/.refactors .

arxiv: https://arxiv.org/abs/2401.01984
medium post: https://medium.com/p/c653ac30e802
GSoC deliverable: https://gist.github.com/jpcbertoldo/12553b7eaa97cfbf3e55bfd7d1cafe88

Closes #1728 1728

✨ Changes

Select what type of change your PR is:

• 🐞 Bug fix (non-breaking change which fixes an issue)
• 🔨 Refactor (non-breaking change which refactors the code base)
• 🚀 New feature (non-breaking change which adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 📚 Documentation update
• 🔒 Security update

✅ Checklist

Before you submit your pull request, please make sure you have completed the following steps:

• 📋 I have summarized my changes in the CHANGELOG and followed the guidelines for my type of change (skip for minor changes, documentation updates, and test enhancements).
• 📚 I have made the necessary updates to the documentation (if applicable).
• 🧪 I have written tests that support my changes and prove that my fix is effective or my feature works (if applicable).

For more information about code review checklists, see the Code Review Checklist.

[jpcbertoldo]

another unresolved issue from the previous PR

[about "ATTENTION..." in docstrings]

@ashwinvaidya17

Same here. We need to consider how these docstrings will be rendered in sphinx.

how can i check that?

[samet-akcay]

another unresolved issue from the previous PR

[about "ATTENTION..." in docstrings]

@ashwinvaidya17

Same here. We need to consider how these docstrings will be rendered in sphinx.

how can i check that?

The documentation is built here based on your changes

[jpcbertoldo]

another unresolved issue from the previous PR
[about "ATTENTION..." in docstrings]
@ashwinvaidya17

Same here. We need to consider how these docstrings will be rendered in sphinx.

how can i check that?

The documentation is built here based on your changes

there are some [metadata] stuff showing up, idk why

apparently sphinx doesnt like dataclasses?
https://anomalib--1726.org.readthedocs.build/en/1726/markdown/guides/reference/metrics/index.html#id5
this field is in the class AUPIMOResult but it's showing as if it was a function or w/e in the root (?)
while AUPIMOResult doesnt show at all

[samet-akcay]

another unresolved issue from the previous PR
[about "ATTENTION..." in docstrings]
@ashwinvaidya17

Same here. We need to consider how these docstrings will be rendered in sphinx.

how can i check that?

The documentation is built here based on your changes

there are some [metadata] stuff showing up, idk why

apparently sphinx doesnt like dataclasses? https://anomalib--1726.org.readthedocs.build/en/1726/markdown/guides/reference/metrics/index.html#id5 this field is in the class AUPIMOResult but it's showing as if it was a function or w/e in the root (?) while AUPIMOResult doesnt show at all

Tree structure is also messed up a bit. It might be an idea to split each metric into a separate section.

[jpcbertoldo]

another unresolved issue from the previous PR
[about "ATTENTION..." in docstrings]
@ashwinvaidya17

Same here. We need to consider how these docstrings will be rendered in sphinx.

how can i check that?

The documentation is built here based on your changes

https://anomalib--1726.org.readthedocs.build/en/1726/markdown/guides/reference/metrics/index.html

it's not quite working as expected

1. i expected per_image to show as submenu in metrics, how could I do that?

2. it seems not to like dataclasses; there are attributes of PIMOResult and AUPIMOResult showing as if it was a function (?) and the classes themselves dont' show

[ashwinvaidya17]

A lot has changed since this PR was submitted, but I've finally gotten around to reviewing it. This is a huge PR with a lot of efforts behind it. However, I have some concerns. I've gone over it once but I think I'll require a few more passes for a more thorough review. But meanwhile we can start the discussions for the current opens.

[jpcbertoldo]

Thanks! I think we are almost there. I have two major comments remaining. First, the author tag in the headers is not consistent with the rest of the repo. The current scans check third-party-programs.txt to verify the license and proper attribution for codes taken from other repositories. Second, numba adds complexity to the codebase. If it is not strictly necessary, let's not include it. But I am happy to hear other opinions.

I removed the author tag and add the original code's repo to third-party-programs.txt.

I think the decision for numba is rather up to y`all as maintainers, but it does give a nice speedup.

For the record (if i get it well), it only has two requirements:

install_requires = [
    'llvmlite >={},<{}'.format(min_llvmlite_version, max_llvmlite_version),
    'numpy >={},<{}'.format(min_numpy_run_version, max_numpy_run_version),
    'importlib_metadata; python_version < "3.9"',
]

https://github.com/numba/numba/blob/d4460feb8c91213e7b89f97b632d19e34a776cd3/setup.py#L369-L373

min_numpy_run_version = "1.22"
max_numpy_run_version = "1.27"
min_llvmlite_version = "0.41.0dev0"
max_llvmlite_version = "0.42"

https://github.com/numba/numba/blob/d4460feb8c91213e7b89f97b632d19e34a776cd3/setup.py#L25-L28

I copied these from numba==0.58.1, which is the minimum here.

[djdameln]

I'm getting the following error when I try to use the AUPIMO metric to evaluate an anomalib model. Can you check it out?

ValueError: The `.compute()` return of the metric logged as 'pixel_AUPIMO' must be a tensor. Found (PIMOResult(shared_fpr_metric='mean-per-image-fpr'), AUPIMOResult(shared_fpr_metric='mean-per-image-fpr', fpr_lower_bound=1e-05, fpr_upper_bound=0.0001, num_threshs=48480))

[djdameln]

I'm also a bit hesitant to add the numba requirement. I can see the benefit that it brings, but at the same time it adds an unnecessary dependency and it increases the complexity of the code. Without Numba we could have a pure pytorch implementation of the metric, which would be much cleaner and more in line with the rest of the library.

[jpcbertoldo]

I'm getting the following error when I try to use the AUPIMO metric to evaluate an anomalib model. Can you check it out?

ValueError: The `.compute()` return of the metric logged as 'pixel_AUPIMO' must be a tensor. Found (PIMOResult(shared_fpr_metric='mean-per-image-fpr'), AUPIMOResult(shared_fpr_metric='mean-per-image-fpr', fpr_lower_bound=1e-05, fpr_upper_bound=0.0001, num_threshs=48480))

This is normal because aupimo returns many values so it was encapsulated in that dataclass.
We could create an option in the torchmetrics interface to optionally return this and (by default) return just the average aupimo instead.
Sounds good?

[djdameln]

This is normal because aupimo returns many values so it was encapsulated in that dataclass.
We could create an option in the torchmetrics interface to optionally return this and (by default) return just the average aupimo instead.
Sounds good?

I think that would be a good idea. With the default setting the metric should be fully compatible with Anomalib's pipeline. So users should be able to enable it from the config/cli or API to have Anomalib report the average AUPIMO value.

[jpcbertoldo]

This is normal because aupimo returns many values so it was encapsulated in that dataclass.
We could create an option in the torchmetrics interface to optionally return this and (by default) return just the average aupimo instead.
Sounds good?

I think that would be a good idea. With the default setting the metric should be fully compatible with Anomalib's pipeline. So users should be able to enable it from the config/cli or API to have Anomalib report the average AUPIMO value.

@djdameln Done : )

i think the only missing issue is about numba (here #1726 (comment))

i can remove it if that's better, but it is already optional like you suggested in your last comment

[jpcbertoldo]

@samet-akcay could you launch a code check here please?

About CDO, I think it is ok to apply the correction? But I'm not 100% sure (don't want to mess up the commit history 😬 )

[samet-akcay]

@samet-akcay could you launch a code check here please?

About CDO, I think it is ok to apply the correction? But I'm not 100% sure (don't want to mess up the commit history 😬 )

done

[samet-akcay]

@jpcbertoldo, thanks for creating this huuuge and amazing PR! Also, thanks for your patience. Took me a while to go through this.

I've got some comments/questions.

Clarity vs Conciseness in Naming

• When reading the code, I would prefer clarity over conciseness. I find the abbreviations hard to follow, which overall slows me down reading the code. Overall, I would prefer full words over abbreviations. Something like;
  classification_curve.py or binary_classification_curve.py
thresholds instead of threshs

Validation

• I'm wondering how important the validation stuff for this metric evaluation?
  • Is it possible to carry out these checks multiple times? Or just in the correct place.
  • Is it possible to store validators in more organized way. currently there is a _validation.py, but there are quite a bit of validators scattered across the sub-package.

Structure of the sub-package

• I feel we could organise the sub-package a bit more. For example, instead of placing multiple modules horizontally like pimo.py, pimo_numpy.py, we could potentially create a pimo sub package that organises these, which would be easier to follow. Similarly, it might be an idea to create something like utils/... , even maybe curves/..?

• For example, the actual PIMO implementation starts in Line 700ish. As a reader, I would expect to see AUPIMO(Metric) in aupimo.py and PIMO(Metric) in pimo.py almost right after imports. The rest of the stuff is sort of util to me

I'm working on something similar in PR #2305 to structure sub packages in data, for easier navigation

data
├── __init__.py
├── dataclasses
│   ├── __init__.py
│   ├── generic.py
│   ├── numpy
│   │   ├── __init__.py
│   │   ├── base.py
│   │   ├── depth.py
│   │   ├── image.py
│   │   └── video.py
│   └── torch
│       ├── __init__.py
│       ├── base.py
│       ├── depth.py
│       ├── image.py
│       └── video.py
├── datamodules
│   ├── __init__.py
│   ├── base
│   │   ├── __init__.py
│   │   ├── image.py
│   │   └── video.py
│   ├── depth
│   │   ├── __init__.py
│   │   ├── folder_3d.py
│   │   └── mvtec_3d.py
│   ├── image
│   │   ├── __init__.py
│   │   ├── btech.py
│   │   ├── folder.py
│   │   ├── kolektor.py
│   │   ├── mvtec.py
│   │   └── visa.py
│   └── video
│       ├── __init__.py
│       ├── avenue.py
│       ├── shanghaitech.py
│       └── ucsd_ped.py
├── datasets
│   ├── __init__.py
│   ├── base
│   │   ├── __init__.py
│   │   ├── depth.py
│   │   ├── image.py
│   │   └── video.py
│   ├── depth
│   │   ├── __init__.py
│   │   ├── folder_3d.py
│   │   └── mvtec_3d.py
│   ├── image
│   │   ├── __init__.py
│   │   ├── btech.py
│   │   ├── folder.py
│   │   ├── kolektor.py
│   │   ├── mvtec.py
│   │   └── visa.py
│   └── video
│       ├── __init__.py
│       ├── avenue.py
│       ├── shanghaitech.py
│       └── ucsd_ped.py
├── ...
├── transforms
│   └── ...
├── utils
│   └── ...
└── validators
    ├── __init__.py
    ├── numpy
    │   ├── __init__.py
    │   ├── depth.py
    │   ├── image.py
    │   └── video.py
    ├── path.py
    └── torch
        ├── __init__.py
        ├── depth.py
        ├── image.py
        └── video.py

Given your other commitments these days, one possibility would be to work on it together by merging it to a feature branch? Any thoughts? @jpcbertoldo, @ashwinvaidya17, @djdameln ?

[samet-akcay]

I think extensions would be sufficient in this case

Suggested change

	accepted_extensions: tuple[str, ...] | None = None,
	extensions: tuple[str, ...] | None = None,

[samet-akcay]

Suggested change

	accepted_extensions (tuple[str, ...] | None): Accepted extensions for the path. An exception is raised if the
	extensions (tuple[str, ...] | None): Accepted extensions for the path. An exception is raised if the

[samet-akcay]

Suggested change

	if accepted_extensions is not None and path.suffix not in accepted_extensions:
	if extensions is not None and path.suffix not in extensions:

[samet-akcay]

I have a question regarding .binclf: Is there any case where the classification is not binary? If binary only, can we assume that the abbreviation bin is redundant?

During reading the code, I find it a bit hard to follow these abbreviations

[samet-akcay]

I think expanding the docstring would be good to explain what each choice would do. For example, why would a user, choose given over minmax_linspace, or mean_fpr_optimized

[samet-akcay]

same as above

[samet-akcay]

why not
HIGH, instead of HI, and LOW, instead LO?

[samet-akcay]

As a reader/user, I would love to see an example to understand how to use this

[samet-akcay]

would it be an idea to move these validation utils to _validate.py? these functions already use functions from validate.

[samet-akcay]

I think examples section here would be really useful. Like what do I need to run AUPIMO with some basic torch input and output

[ashwinvaidya17]

I agree with Samet’s suggestion to move all validation checks to a separate module, as it would significantly improve the clarity and organization of pimo.py. Furthermore, I propose we also consider removing the NumPy-based computations. Since we aren’t using NumPy for other metrics in Anomalib, and there doesn’t seem to be a current need for NumPy in our metric computations, eliminating it could further simplify the codebase.
As Samet suggested, we can target this change to a feature branch so that the rest of the team can assist in reducing your workload, especially given the significant amount of work you’ve already done.

[jpcbertoldo]

I agree with Samet’s suggestion to move all validation checks to a separate module, as it would significantly improve the clarity and organization of pimo.py. Furthermore, I propose we also consider removing the NumPy-based computations. Since we aren’t using NumPy for other metrics in Anomalib, and there doesn’t seem to be a current need for NumPy in our metric computations, eliminating it could further simplify the codebase. As Samet suggested, we can target this change to a feature branch so that the rest of the team can assist in reducing your workload, especially given the significant amount of work you’ve already done.

Ok, no worries. So shoul I remove the *_numpy.py stuff and move the code to the torch onlye versions ?

Just one warning. PIMO is fast to compute thanks to

https://github.com/jpcbertoldo/anomalib/blob/241df0e2aea82373bb69c216e73a7e43106aee22/src/anomalib/metrics/per_image/binclf_curve_numpy.py#L98

which is in numpy, so that specific one would be worth keeping.

Using torchmetrics (even on GPU) is slower. The constraint is that the thresholds are shared across images, but the binary classification (for multiple thresholds) are computed per image.

If this one is removed, then we need a torch-based implementation, which I'm sure is significantly slower for evaluation on the original resolution of images (1000-ish), which was a major argument in the paper. This is a topic we went through last year, but I can try to find the graphs to put numbers in this difference of execution time.

So, do we keep or remove this one? (_binclf_one_curve_python)

[ashwinvaidya17]

Okay, then let's keep bin_clfcurve_numpy.py. I wonder how will this affect the computation when we support distributed training. Anyway, we can look into that later. Let's get this merged.

[jpcbertoldo]

Okay, then let's keep bin_clfcurve_numpy.py. I wonder how will this affect the computation when we support distributed training. Anyway, we can look into that later. Let's get this merged.

(just guessing) In the torchmetrics class, there isn't much happening in the update(), it basically stores score maps and masks. The compute() method will do everything. It' pretty much what torchmetrics does with AUROC. So it should be ok?