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

Enable any resolution for Unet #1029

Merged
merged 7 commits into from
Jan 13, 2025
Merged

Enable any resolution for Unet #1029

merged 7 commits into from
Jan 13, 2025

Conversation

qubvel
Copy link
Collaborator

@qubvel qubvel commented Jan 11, 2025

What does this PR do?

  1. Replace scale_factor with size argument in Unet DecodeBlock to enable forward with any image resolution (not only divisible by 32)
  2. Improve Unet docstring, add example
  3. Fix type hint for models (was bugged by double inheritance with HubMixin)
  4. Use inference_mode instead of no_grad in tests

Copy link

codecov bot commented Jan 11, 2025

Codecov Report

Attention: Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...gmentation_models_pytorch/decoders/unet/decoder.py 96.29% 1 Missing ⚠️
Files with missing lines Coverage Δ
segmentation_models_pytorch/base/model.py 81.57% <100.00%> (+2.79%) ⬆️
segmentation_models_pytorch/decoders/unet/model.py 100.00% <100.00%> (ø)
...gmentation_models_pytorch/decoders/unet/decoder.py 91.37% <96.29%> (+0.99%) ⬆️

@qubvel qubvel requested a review from adamjstewart January 11, 2025 20:33
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Not sure if I'm familiar enough with the details of the model to properly review this

):
super().__init__()
self.interpolate_mode = interpolation_mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit confusing to me to see both "interpolate" and "interpolation", maybe we can make these consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the catch! will fix it

@JulienMaille
Copy link
Contributor

Could this break compatibility with onnx export -> OpenCV inference?

@qubvel
Copy link
Collaborator Author

qubvel commented Jan 12, 2025

@JulienMaille thanks for looking into it!

Right, initially, there was an issue with ONNX export, but nowadays ONNX export works with the size argument as well. I will check this out manually, but I plan to add ONNX exportability tests for all models.

@qubvel
Copy link
Collaborator Author

qubvel commented Jan 13, 2025

@JulienMaille I just checked this; the minimum opset to export the model with size is 11, while currently, the latest one is 21, so we are good to go

@qubvel qubvel merged commit 93b19d3 into main Jan 13, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants