-
Notifications
You must be signed in to change notification settings - Fork 709
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
Add CFA model #783
Add CFA model #783
Conversation
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.
Thanks for the efforts! I have a few minor comments.
|
||
### Image-Level AUC | ||
|
||
| | ResNet-18 | Wide ResNet50 | |
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.
Minor issue but we use a horizontal layout in other models.
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.
yeah, but we need to scroll to the side when we have the horizontal layout. I think this one looks better than the old one. If you guys also agree, we could maybe change the layout for other models?
normalize: bool = True, | ||
border_type: str = "reflect", | ||
padding: str = "same", | ||
) -> None: | ||
"""Initialize model, setup kernel etc.. | ||
|
||
Args: | ||
kernel_size (Union[Tuple[int, int], int]): size of the Gaussian kernel to use. | ||
sigma (Union[Tuple[float, float], float]): standard deviation to use for constructing the Gaussian kernel. |
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.
Not part of this PR but should we move these init_args above to make it consistent with other classes?
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 few comments, all minor
Returns: | ||
int: Kernel size. | ||
""" | ||
return 2 * int(4.0 * sigma_val + 0.5) + 1 |
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.
We use the same computation in Padim (here), and maybe other models as well. Would be good to use this function there as well.
Co-authored-by: Dick Ameln <[email protected]>
…ib into feature/model/cfa
This reverts commit 7752bb3.
Add CFA model (openvinotoolkit#783)
Description
TODO:
Changes
Checklist