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

Add ensembling methods for tiling to Anomalib #1226

Merged
merged 242 commits into from
Oct 24, 2024

Conversation

blaz-r
Copy link
Contributor

@blaz-r blaz-r commented Aug 1, 2023

Description

This PR adds mechanism to train ensemble of models on tiled images. It is part of Google Summer of Code.

Implementation Details:

Closes:

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

Some things still todo (tests, docs...) but most of the code is ready for review.

  • 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
  • I have added a summary of my changes to the CHANGELOG (not for minor changes, docs and tests).

@samet-akcay
Copy link
Contributor

@blaz-r the pre-commit checks are failing due to some comments left out in the code. Would you want to address them. I will need to merge this PR this week, as I would like this to be part of 1.2, which I plan to release next week.

@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 22, 2024

Okay, I'll handle the remaining things in a few hours.

@samet-akcay
Copy link
Contributor

Great, thanks!

blaz-r and others added 5 commits October 22, 2024 19:05
Signed-off-by: blaz-r <blaz.rolih@gmail.com>
Signed-off-by: blaz-r <blaz.rolih@gmail.com>
Signed-off-by: blaz-r <blaz.rolih@gmail.com>
Signed-off-by: blaz-r <blaz.rolih@gmail.com>
@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 22, 2024

The issues should now be resolved.

@samet-akcay
Copy link
Contributor

Thanks @blaz-r, I've just triggered the CI

@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 22, 2024

It's failing on tiled ensemble "predict" pipeline integration test, that is executed after "train" pipeline. I'm not sure why, but it looks like the weight file is not found. Could be due to file name, but I don't know how to debug this, as it works fine locally (of course 😅 )

Signed-off-by: blaz-r <blaz.rolih@gmail.com>
@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 22, 2024

I believe the problem is in file path case (since windows is case insensitive 🙄). I changed that now.

@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 22, 2024

Eh, still not quite okay. Let me sort this locally on WSL.

Signed-off-by: blaz-r <blaz.rolih@gmail.com>
@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 22, 2024

Okay, I think it is now resolved (at least for me on both Windows and WSL ubuntu). For some reason, temporary directory behaves differently on the Linux, but I solved that by using the existing "project_path" fixture, and it seems to work alright now.

@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 23, 2024

It looks like it's still failing due to file not found. I'm really not sure why that is happening at this point. The weight file works just fine on my end, both in Windows and Linux. It might be possible that there's an error with the temporary directory, or the file name, but I'm not sure how to debug that inside actions.

@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 23, 2024

It could be a problem of case, since mvtec path could be named "MVTec", but looking at other tests they use small case as well. In any case, it works just fine on Ubuntu for me. I can change it to "MVTec" and we run again, or if you have any other ideas how to resolve this.

Signed-off-by: Blaz Rolih <blaz.rolih@gmail.com>
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 96.63182% with 29 lines in your changes missing coverage. Please review.

Project coverage is 81.66%. Comparing base (3a403ae) to head (8e3d9d5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nes/tiled_ensemble/components/stats_calculation.py 93.05% 5 Missing ⚠️
...nomalib/pipelines/tiled_ensemble/train_pipeline.py 87.80% 5 Missing ⚠️
...s/tiled_ensemble/components/metrics_calculation.py 96.55% 3 Missing ⚠️
...elines/tiled_ensemble/components/model_training.py 95.23% 3 Missing ⚠️
.../pipelines/tiled_ensemble/components/prediction.py 96.29% 3 Missing ⚠️
...anomalib/pipelines/tiled_ensemble/test_pipeline.py 92.50% 3 Missing ⚠️
...b/pipelines/tiled_ensemble/components/smoothing.py 97.10% 2 Missing ⚠️
...ed_ensemble/components/utils/prediction_merging.py 96.36% 2 Missing ⚠️
...pelines/tiled_ensemble/components/visualization.py 96.07% 2 Missing ⚠️
...lib/pipelines/tiled_ensemble/components/merging.py 97.61% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
+ Coverage   80.38%   81.66%   +1.27%     
==========================================
  Files         264      283      +19     
  Lines       11824    12682     +858     
==========================================
+ Hits         9505    10357     +852     
- Misses       2319     2325       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samet-akcay
Copy link
Contributor

@blaz-r it's all green! ✅

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 a ton for the huge effort!

@blaz-r
Copy link
Contributor Author

blaz-r commented Oct 23, 2024

Great 😄 🎉

Copy link
Contributor

@djdameln djdameln left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this very nice implementation! This is a great example of how the pipelines API can be used to achieve complex custom workflows.

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.

Thanks for the massive efforts!

@samet-akcay
Copy link
Contributor

Merging now. Thanks again!

@samet-akcay samet-akcay merged commit db4c285 into openvinotoolkit:main Oct 24, 2024
7 checks passed
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.

6 participants