-
Notifications
You must be signed in to change notification settings - Fork 46
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
Support segmentation masks #87
Conversation
To not break your dev workflow, I made a PR to barucden#1, you can review and merge if that looks good to you. Ideally, we could do this in |
I had to explicitly define the type of |
Looking good to me! Fighting with method ambiguity isn't a very interesting journey, do you think we need to introduce some new function e.g., |
I am not sure. I can imagine that the ambiguity is an issue if more conventions (like the pair notation) are introduced. But do you think there will be more? As of now, I think it is manageable. |
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 had to explicitly define the type of img for all augment methods to remove ambiguities. I hope it's fine.
In barucden#2 I rewrite the augment
to remove the ImageOrTuple
constraint
There was also an issue with |
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.
done. welcome! |
@barucden merge when you're ready. |
@@ -56,9 +60,18 @@ for FUN in (:applyeager, :applylazy, :applypermute, | |||
param = randparam(op, imgs) | |||
map(img -> ($FUN)(op, img, param), imgs) |
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'm trying to wrap my head around the implementation. Before this commit, imgs
contained images of the same datatype, after, it contains AbstractArrays and Masks. So inside the call to map()
dynamic dispatch is triggered (?). Is this a problem? I think this could be avoided somehow by changing codegen.jl to specialize / optimize the given pipeline twice: once for the image, once for the masks. Or is this already happening with this implementation? All the augment_impl
code looks a bit like black magic to me.
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.
So inside the call to map() dynamic dispatch is triggered.
No, it won't. That's why I introduced Val{false}
and Val{true}
so that the true
/false
information can be passed to Julia at compiled time.
You can verify this via @inferred
or @code_warntype
.
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.
Also we can benchmark this:
using Augmentor
img = Augmentor.testpattern()
@btime augment(($img, $img), Scale(0.5));
# this PR: 1.262 ms (60 allocations: 243.34 KiB)
# master: 1.318 ms (60 allocations: 243.34 KiB)
@btime augment(($img, $(Augmentor.Mask(img))), Scale(0.5));
# 1.262 ms (60 allocations: 243.34 KiB)
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.
So the map-over-tuple seems to be unrolled during compilation, nice :)
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.
If there are runtime dispatch, usually there are suspicious memory allocations, e.g.,
julia> rand01_stable() = rand() > 0.5 ? 0 : 1
rand01_stable (generic function with 1 method)
julia> rand01_unstable() = rand() > 0.5 ? 0.0 : 1
rand01_unstable (generic function with 1 method)
julia> g(f) = [f() for _ in 1:100]
g (generic function with 2 methods)
julia> @btime g(rand01_stable);
182.425 ns (1 allocation: 896 bytes)
julia> @btime g(rand01_unstable);
1.031 μs (46 allocations: 2.44 KiB)
Another new tool that I haven't yet understand well is JETTest
julia> @report_dispatch augment((img, img), Scale(0.5))
No errors !
(Tuple{Matrix{ColorTypes.RGBA{FixedPointNumbers.N0f8}}, Matrix{ColorTypes.RGBA{FixedPointNumbers.N0f8}}}, 0)
julia> @report_dispatch augment((img, Augmentor.Mask(img)), Scale(0.5))
No errors !
(Tuple{Matrix{ColorTypes.RGBA{FixedPointNumbers.N0f8}}, Augmentor.Mask{Matrix{ColorTypes.RGBA{FixedPointNumbers.N0f8}}}}, 0)
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.
So the map-over-tuple seems to be unrolled during compilation, nice :)
Yes. It's actually Tuple{typeof(img1), typeof(img2), ...}
so Julia fully gets the type information to do dispatch at compile time. But if it is a very long tuple(say 100), then Julia compilation would just blow up and take forever to compile...
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.
Was it actually required that imgs
contained images of the same datatype?
So inside the call to map() dynamic dispatch is triggered (?).
Yes. For ordinary images, the methods that already were there (applyeager
, applylazy
, etc.) are called -- so nothing changes in that case. For semantic wrappers these methods are called, which then determine if the given operation should be applied and use dispatching once more to apply/skip the operation. I am not sure if any of that is a problem.
changing codegen.jl to specialize / optimize the given pipeline twice: once for the image, once for the masks
It is hard for me to understand the code in codegen.jl
too. However, I believe that the pipelines are optimized w.r.t. the included operations, not the input. Also, it would perhaps require bigger changes to optimize the pipeline separately for images and for masks because we need to apply the same operations on both images and masks (if applicable) so that the respective pixels still correspond after the augmentation. So the rough approach is
for operation in pipeline
produce operation's parameters
apply operation on each input
not
for image in input
augment image using all operations in pipeline
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 believe that the pipelines are optimized w.r.t. the included operations, not the input.
Correct. The idea is to collapse multiple operations into one so that computation goes faster. Say,
- (affine optimization): applying two coordinate transformations can be collapsed into composing two transformation matrices (which is at most 3*3 matrix multiplication)
- (lazy): applying two lazy operations can be done via broadcasting so that we can reduce intermediate memory allocation.
This Mask
decorates the input data, which is not what codegen currently cares about.
Also, it would perhaps require bigger changes to optimize the pipeline separately for images and for masks
Because NoOp has the best property that we can take advantage of. One possible way/hack is to add one more pass to change unactivated operations to NoOp before the current code_gen
. With this extra pass, seek_connected
can get better results to optimize. But this would require a second pipeline internally to make all those happen.
@Evizero thank you for the invitation! @johnnychen94 I am thinking I should add one more test to check all the remaining operations cooperate with semantic wrappers. Then I think it is safe to merge. |
One last API thing: I think it is worthwhile to implement an augment version for named tuples, while you're at it. That would be super clear for users and I think also extensible. But either way, I'm already super happy with the current implementation - thanks for the effort! |
Since we only support masks now, would this API bring something "new"? Seems like a more verbose version to the pair version: augment(img => mask, pl) # can do now
augment((image=img, mask=mask), pl) # named tuple version I can see it being useful once we support more targets (besides masks), so maybe we should decide the API when we implement additional targets? |
Agreed. Adding support for NamedTuple can be considered when there is a new application that this PR doesn't cover. By then it can be easier to provide a justified API. |
7e37088
to
f24c5ae
Compare
Co-authored-by: Johnny Chen <[email protected]>
Thanks again mate! It‘s just a small PR, but a big step for julia adoption in my work :) |
Glad to hear! Give it a try. It certainly needs some "battle testing" :) |
@barucden in most cases, we follow the continuous release strategy. So I've made a patch release ad9a398 https://www.oxinabox.net/2019/09/28/Continuous-Delivery-For-Julia-Packages.html |
Oh, okay, got it! Thanks |
This PR introduces support for segmentation masks. The key principle is that we do not apply some operations on segmentation masks.
That being said, the implementation is not finished yet, and I would like to kindly ask for assistance. I am not sure how to implement the "operation skipping", or perhaps where to implement it. I am thinking maybe operation.jl#L57 and operation.jl#L60 so it would result in
But I am not convinced.
TODO:
SemanticWrapper
interfaceCloses #85