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

feat: kornia binary morph ops #428

Merged
merged 8 commits into from
Aug 1, 2023
Merged

feat: kornia binary morph ops #428

merged 8 commits into from
Aug 1, 2023

Conversation

nkemnitz
Copy link
Collaborator

No description provided.

@nkemnitz
Copy link
Collaborator Author

@supersergiy Do you want tests here or is that more of a delegation thing?
Also, any reason to keep the custom binary_closing around?

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #428 (8b6a13f) into main (0525c92) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #428   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          120       120           
  Lines         3588      3618   +30     
=========================================
+ Hits          3588      3618   +30     
Files Changed Coverage Δ
zetta_utils/tensor_ops/mask.py 100.00% <100.00%> (ø)

Copy link
Member

@supersergiy supersergiy left a comment

Choose a reason for hiding this comment

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

This looks really cool!

Although the new functions don't have if/for statements, they're reasonably complex and would benefit from testing. The upside is that because there's no if's, each function will need only a single test case to be fully covered.

Re deprecation:
The kornia_closing requires kernel to be passed in explicitly, while the "custom" binary_closing just assumes something reasonable. The explicit kernel is useful in some scenarios, but making it a required argument will make it a hassle a lot of the time. I think ideally we'd make the kernel an optional argument, which would let us deprecate the non-kornia alternatives.

Also, the kernerl argument should probably be typed, unless there's something I'm missing.

@nkemnitz nkemnitz force-pushed the nkem/feat-morph-kornia branch 2 times, most recently from 39bc60a to 7596599 Compare July 6, 2023 21:37
@nkemnitz
Copy link
Collaborator Author

Set default kernel to square footprint of width=3, removed unnecessary conversion to floating point, added documentation

Copy link
Member

@supersergiy supersergiy 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! It's great that the kernel typing is more precise now - Tensor instead of TensorTypeVar. Now that the new kornia based operations have width, should we deprecate the old coarsen and dilation too?

@supersergiy supersergiy merged commit 32d4243 into main Aug 1, 2023
10 checks passed
@nkemnitz nkemnitz deleted the nkem/feat-morph-kornia branch August 1, 2023 21:13
supersergiy added a commit that referenced this pull request Feb 2, 2024
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.

2 participants