Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PIMO #1726
PIMO #1726
Changes from 53 commits
c3944eb
b61c47f
e6006b4
101d646
62d5480
0f0b424
8df211e
9e74226
283d704
2bc3c06
c864b54
a3d1060
bfc287e
60483fc
4bfe3da
1a7398b
403b4ae
b12fb86
b7e5439
3808de8
dfa8dc3
2cefa2c
adc14fd
c30c4ea
408fb2b
43c6eb2
980c972
a052f6a
dabba4a
215847b
a6404fc
14c97fa
5bc3b2b
b8e0ddf
943c1a7
2e565d1
103b6db
08b85ef
6165768
fbdf8b6
f558904
581b35b
5837c0d
68a30aa
d4071ad
2f65040
0d0863f
fdd797a
012e8e2
f11b4a9
fa448f2
dd0bd50
c92a6a9
1cde9ce
be9732f
0bef471
440bf2f
937a515
7c037ec
718257c
27a38da
241df0e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think extensions would be sufficient in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question regarding
.binclf
: Is there any case where the classification is not binary? If binary only, can we assume that the abbreviationbin
is redundant?During reading the code, I find it a bit hard to follow these abbreviations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still on the fence regarding the numba requirement. While it is a good feature to have, it increases complexity. I would recommend dropping it from Anomalib. Any user who wants to use it can refer to the original repo. Any thoughts @samet-akcay @djdameln ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the pytorch-only was slower than the one with numpy (which is slower than numba)
i see this is becoming a long issue, i can just drop numba, but can you confirm this is the decision to make? (just to avoid going back later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpcbertoldo would it be possible to use the pytorch-only implementation by default, and use the Numba implementation if the user has Numba installed in their environment? This way we don't force casual users to install another dependency, and advanced users who care more about speed can install Numba to speed up the computation. We could log a warning to notify the users that the computation can be made faster by installing Numba.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djdameln this is already the case
this
anomalib/src/anomalib/metrics/per_image/binclf_curve_numpy.py
Lines 23 to 32 in 440bf2f
and this part
anomalib/src/anomalib/metrics/per_image/binclf_curve_numpy.py
Lines 229 to 239 in 440bf2f
the installation of numba is already optional as well and it requires "opt-in" from the user:
anomalib/pyproject.toml
Line 87 in 440bf2f
@ashwinvaidya17 do you think that is ok like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samet-akcay can we give a final push to decide about keeping
numba
or not?i think it is the only thing blocking the merge
the code doesn't depend on it, for the moment it is optional to install/use it, so the question is if we keep it or just get rid of it