Skip to content
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

padim arguments improvements #664

Conversation

jpcbertoldo
Copy link
Contributor

@jpcbertoldo jpcbertoldo commented Nov 1, 2022

Description

Fix #662 by creating the parameter n_features (nb of features/axes retained by the dimension reduction).

I also made it possible to use a custom backbone without modifying the source code.
This was limited by this hardcoded dict

image

If the backbone was not in the dict then the orig_dims and emb_scale could not be set.
These two don't need to be hardcoded, I made it be deduced from a dryrun feature extraction.

Changes

  • 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)
  • 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

Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jpcbertoldo. The timing of this PR is also great, given that we wanted to add Efficient backbone support.

@jpcbertoldo
Copy link
Contributor Author

Cool!

I will write some the doc for it.
Does this feature need tests in your opinion?

Copy link
Collaborator

@ashwinvaidya17 ashwinvaidya17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@jpcbertoldo jpcbertoldo force-pushed the jpcbertoldo/padim-config-improvements-2 branch from ffb2190 to 9735cd8 Compare November 2, 2022 16:39
@jpcbertoldo
Copy link
Contributor Author

done
i further simplified a bit

…jpcbertoldo/anomalib into jpcbertoldo/padim-config-improvements-2
@github-actions github-actions bot added the Tests label Nov 7, 2022
@jpcbertoldo
Copy link
Contributor Author

Maybe it would be good to add a unit test for this function

@djdameln it didn't feel right to have such a specific test.

Instead i extracted the dryrun part and tested that. The logic in _deduce_dims is trivial now.

@jpcbertoldo jpcbertoldo requested a review from djdameln November 7, 2022 19:46
Copy link
Contributor

@samet-akcay samet-akcay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is going to be merge conflicts between this PR and #675. @ashwinvaidya17 and @jpcbertoldo, can you guys sync how to minimise/avoid these conflicts?

@jpcbertoldo
Copy link
Contributor Author

The conflict is only in anomalib/models/components/feature_extractors/__init__.py right?

Doesnt look so hard, whicever is merged second rebases.

@ashwinvaidya17 could you check if the new function in that file in #675 would work with the torchfx extractor? If not then it needs a subcase.

@ashwinvaidya17
Copy link
Collaborator

The conflict is only in anomalib/models/components/feature_extractors/__init__.py right?

Doesnt look so hard, whicever is merged second rebases.

@ashwinvaidya17 could you check if the new function in that file in #675 would work with the torchfx extractor? If not then it needs a subcase.

I am working on a design to make the feature extractor configurable. I think we can move dimension computation into the feature extractor class. Since the design might take time we can merge this one first and then I can update PR #675

@jpcbertoldo
Copy link
Contributor Author

The conflict is only in anomalib/models/components/feature_extractors/__init__.py right?
Doesnt look so hard, whicever is merged second rebases.
@ashwinvaidya17 could you check if the new function in that file in #675 would work with the torchfx extractor? If not then it needs a subcase.

I am working on a design to make the feature extractor configurable. I think we can move dimension computation into the feature extractor class. Since the design might take time we can merge this one first and then I can update PR #675

Cool : )

@jpcbertoldo jpcbertoldo requested review from samet-akcay and removed request for djdameln November 8, 2022 13:00
@samet-akcay samet-akcay merged commit 15625d6 into openvinotoolkit:main Nov 8, 2022
@jpcbertoldo jpcbertoldo deleted the jpcbertoldo/padim-config-improvements-2 branch February 9, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

padim should allow the user to modify the embedding size
4 participants