-
Notifications
You must be signed in to change notification settings - Fork 615
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
Operator RandomObjectBBox #2657
Conversation
!build |
CI MESSAGE: [2056699]: BUILD STARTED |
This pull request introduces 1 alert when merging 5402c7dad5d57cca5a0cf3e47658c4aae626682f into 2c0b597 - view on LGTM.com new alerts:
|
CI MESSAGE: [2056699]: BUILD FAILED |
a3a912a
to
d99fa00
Compare
!build |
CI MESSAGE: [2058334]: BUILD STARTED |
CI MESSAGE: [2058334]: BUILD FAILED |
!build |
CI MESSAGE: [2058874]: BUILD STARTED |
CI MESSAGE: [2058877]: BUILD STARTED |
CI MESSAGE: [2058877]: BUILD PASSED |
CI MESSAGE: [2058874]: BUILD PASSED |
context.classes.push_back(cls); | ||
context.weights.push_back(1); | ||
} | ||
std::sort(context.classes.begin(), context.classes.end()); |
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.
Sorting is necessary, because the order of elements in unordered set is not only undefined with respect to current contents, but also depends on previous contents (before clear) - as this changes the number of bins.
It's cheaper to copy it from unordered set and sort, because unordered set has better memory management than set
and we need a copy to a flat array anyway.
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 would be for writing such comments directly in the code instead of the PR review.
This operator takes a labeled segmentation map as its input. With probability ``foreground_prob`` | ||
it randomly selects a label (uniformly or according to the distribution given as ``class_weights``), | ||
extracts connected blobs of pixels with the selected label and randomly selects one of them | ||
(with additional constraints given as ``k_largest`` and ``threshold``). |
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 feel the content of this parenthesis deserves its own sentence. It is not clear enough now.
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'll try to rephrase it.
it randomly selects a label (uniformly or according to the distribution given as ``class_weights``), | ||
extracts connected blobs of pixels with the selected label and randomly selects one of them | ||
(with additional constraints given as ``k_largest`` and ``threshold``). | ||
With probability 1-foreground_prob, entire area of the input is returned.)") |
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.
With probability 1-foreground_prob, entire area of the input is returned.)") | |
With probability 1-foreground_prob, the entire area of the input is returned.)") |
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.
Maybe also add info of what are the outputs (and possible formats?)
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 don't know if it's wort repeating here.... I'll take a look.
return 1 + separate_corners + output_class; | ||
}) | ||
.AddOptionalArg("ignore_class", R"(If True, all objects are picked with equal probability, | ||
regardless of the class they belong to. Otherwise, a class is picked first and then object is |
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.
regardless of the class they belong to. Otherwise, a class is picked first and then object is | |
regardless of the class they belong to. Otherwise, a class is picked first and then the object is |
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.
an object
This flag only affects the probability with which blobs are selected. It does not cause | ||
blobs of different classes to be merged.)", false) | ||
.AddOptionalArg("output_class", R"(If True, an additional output is produced which contains the | ||
label of the class to which the resulting box belongs, or background label if foreground box |
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.
label of the class to which the resulting box belongs, or background label if foreground box | |
label of the class to which the resulting box belongs, or the background label if the selected box |
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.
true that
void RandomObjectBBox::GetClassesAndWeightsArgs( | ||
ClassVec &classes, WeightVec &weights, int &background, int sample_idx) { | ||
background = background_[sample_idx].data[0]; | ||
if (ignore_class_) |
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 would rather put this if before calling this function.
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.
But then I'd have to get background in the calling function.
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.
Yes, or have a separate getter for the background so that you see:
GetBackground(...)
if (!ignore_label_)
GetClassesAndWeights(...)
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.
It doesn't work like this. If there are classes or weights, background may depend on them.
void StoreBox(const OutListCPU<int, 1> &out1, | ||
const OutListCPU<int, 1> &out2, | ||
RandomObjectBBox::OutputFormat format, | ||
int sample_idx, Box &&box) { |
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.
Just a question: Have you considered using one of the existing structures for Box/ROI/etc?
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.
These structures have fixed number of dimensions whereas this works with dynamic dimensions.
} | ||
if (choose_different_background) { | ||
// This will yield incorrect result if the set of classes contains both INT_MIN and INT_MAX, | ||
// which is a sacrifice I am willing to make. |
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.
can you enforce somewhere that this is not the case?
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 could even loop over and decrement until I find a free slot.... I don't know how much value is there in it.
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 was thinking of a simple assert or DALI_ENFORCE.
.AddOptionalArg<vector<int>>("threshold", R"(Minimum extent(s) of the bounding boxes to return. | ||
|
||
If current class doesn't contain any bounding box that meets this condition, the largest one | ||
is returned.)", nullptr, true) |
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.
Hmmm... I see that I didn't implement that (and just try a different class or fall back to background).
I don't know what I should do in this case. Change implementation or change the docs? Arguably, it makes sense not to return a box below the threshold, even if it's the only one in its class.
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 am OK with that (not returning the box). Given that it is properly documented
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.
Now the documentation follows what happens in the code.
SmallVector<std::pair<int64_t, int>, 32> vol_idx; | ||
vol_idx.resize(n); | ||
for (int i = 0; i < n; i++) { | ||
vol_idx[i] = { -volume(boxes[i]), i }; |
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.
You could leave the volume possitive and use std::greater<> with std::sort
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.
But then I'd have to reverse sign of i
to get stability.
if (n <= 0) | ||
return -1; | ||
|
||
if (k_largest_ > 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.
Small improvement: If your k_largest is bigger than the actual number of objects you don't need to sort either.
|
||
VALUE_SWITCH(ndim, static_ndim, (1, 2, 3, 4, 5, 6), | ||
( | ||
auto *box_data = reinterpret_cast<Box<static_ndim, int>*>(ctx.box_data.data()); |
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.
Wasn't it technically undefined behavior? :)
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 know, but it's sort-of less undefined here, since it crosses function boundary and it's a tad safer to type-pun an array instead of one object.
template <typename T> | ||
bool RandomObjectBBox::PickForegroundBox( | ||
SampleContext &context, const InTensorCPU<T> &input) { | ||
GetClassesAndWeightsArgs(context.classes, context.weights, context.background, |
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'd move this call inside if (ignore_class_)
and remove the check inside GetClassesAndWeights? Maybe have a separate getter for the background
} | ||
std::sort(context.classes.begin(), context.classes.end()); | ||
} else { | ||
for (int i = 0; i < static_cast<int>(context.classes.size()); i++) { |
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.
for (int i = 0; i < static_cast<int>(context.classes.size()); i++) { | |
for (size_t i = 0; i < context.classes.size(); i++) { |
class_label_out = view<int, 0>(ws.OutputRef<CPUBackend>(class_output_idx_)); | ||
|
||
TensorShape<> default_anchor; | ||
default_anchor.resize(ndim); |
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.
default_anchor.resize(ndim); | |
default_anchor.resize(ndim, 0); |
|
||
if (HasClassLabelOutput()) | ||
class_label_out.data[i][0] = ctx.class_label; | ||
}, volume(in_shape.tensor_shape_span(i))); |
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.
}, volume(in_shape.tensor_shape_span(i))); | |
}, in_shape.tensor_size(i)); |
|
||
bool output_class_ = spec.GetArgument<bool>("output_class"); | ||
|
||
if (ignore_class_ && (classes_.IsDefined() || weights_.IsDefined() || output_class_)) { |
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.
OK, so if you have this, you can simply place an assert(!ignore_class)
in the function where you get classes and weights
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.
Then I'd have to duplicate the code to get background value - and the control flow doesn't favor it.
|
||
format = formats[fmt] | ||
fmt = (fmt + 1) % len(formats) | ||
dtype = types[np.random.randint(0, len(types))] |
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.
Use np.random.choice(types)
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.
Actually, now numpy complain that random choice is deprecated for non-numeric types... I'd leave it as-is and fix it in the parallel PR...?
|
||
format = format or "anchor_shape" | ||
|
||
for _ in range(50): |
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.
can you make the number of iterations a parameter of the function?
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 could, but what for?
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.
It's easy to change later if we need to
!build |
CI MESSAGE: [2067103]: BUILD STARTED |
CI MESSAGE: [2067103]: BUILD PASSED |
49fe8d8
to
2ecd968
Compare
!build |
CI MESSAGE: [2070401]: BUILD STARTED |
CI MESSAGE: [2070401]: BUILD PASSED |
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
* Propagate batch size from batch external sources to per-sample external sources * Fix bugs Signed-off-by: Michał Zientkiewicz <[email protected]>
* Add `first = False` in external source handling to produce an error when external sources disagree on batch size. Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
2ecd968
to
e734678
Compare
!build |
CI MESSAGE: [2077424]: BUILD STARTED |
CI MESSAGE: [2077424]: BUILD PASSED |
Signed-off-by: Michał Zientkiewicz <[email protected]>
!build |
CI MESSAGE: [2078838]: BUILD STARTED |
CI MESSAGE: [2078838]: BUILD PASSED |
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 yet to review the tests, but I'm already done with most of the implementation so posting my comments.
The flow looks correct but I had objections to the SampleContext and its API: mainly the fact that the Init will leave it in some weird state and the GetBgFgAndWeights is done a bit from the outside (even though it probably shouldn't access the ArgValues directly, it would make more sense to me if the initialization was done first and left the object in a complete state).
format_ = ParseOutputFormat(spec.GetArgument<string>("format")); | ||
ignore_class_ = spec.GetArgument<bool>("ignore_class"); | ||
|
||
bool output_class_ = spec.GetArgument<bool>("output_class"); |
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.
May be misleading someone into thinking that it is a member variable.
bool output_class_ = spec.GetArgument<bool>("output_class"); | |
bool output_class = spec.GetArgument<bool>("output_class"); |
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.
Will fix.
StoreBox(out1, out2, format_, i, default_anchor, input.tensor_shape(i)); | ||
if (HasClassLabelOutput()) { | ||
SampleContext &ctx = contexts_[0]; | ||
GetBgFgAndWeights(ctx.classes, ctx.weights, ctx.background, i); |
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.
Don't you need to just output the background for this sample. Is the whole GetBgFgAndWeights necessary here? There won't be any further processing done for this sample, right?
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.
Background label may be automatically assigned so as to skip classes
, so I need to process them anyway.
StoreBox(out1, out2, format, sample_idx, box.lo, box.hi); | ||
} | ||
|
||
void RandomObjectBBox::GetBgFgAndWeights( |
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.
As I'm reading this I feel a bit of spaghetti. The context has Init
, but it only resizes some data, than there is this function, which directly modifies context's members (but if ignore_class_ is true I guess it will just leave them in an undefined state, either some leftovers from previous sample or just empty), than the context again has methods to operate on for example weights
, assuming that they come from outside and are valid (and it uses them to further initialize some members).
I'm thinking how to untangle this, maybe I would start with zeroing the classes and weights in the context Init
or having the Init
inspect the classes and weights for that input and initialize those (so do more of GetBgFgAndWeights).
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 admit that it is indeed spaghettish, but I'd prefer to address this in the followup (parallelization PR), as changing this logic here would result in very painful rebasing there - the context object has undergone significant changes between the two.
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 can understand that, if you can take of untangling it a bit in the follow up I would be ok with that under current circumstances.
} | ||
|
||
if (HasClassLabelOutput()) | ||
class_label_out.data[i][0] = ctx.class_label; |
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.
This result passed via ctx from the two possible branches of the if (PickForegroundBox(ctx))
feels a bit weird.
Maybe encapsulate it in StoreLabel(i, ctx.selected_label)/StoreLabel(i, ctx.background) in the if/else respectively.
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 don't think makes much sense to encapsulate a simple assignment in a function - especially given that I'd either have to pass (ugly) or store in the object (uglier) the class_label_out
variable, which is now local.
|
||
context.labels.erase(context.background); | ||
|
||
if (!classes_.IsDefined() && !weights_.IsDefined()) { |
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.
This is the last case, for the GetBgFgAndWeights
, but it's not handled there. I understand that you need to gather the labels first, but than, why not gather them in the Init? You're already inspecting the input a bit.
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.
Init is called in the main thread and I definitely don't want to scan entire input there.
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.
For the fg
case (else), where you add the tasks to thread pool it's already in the lambda and I don't see any other invocation.
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.
Indeed. However, I don't need to call init
at all for the background case - I don't need the temporary storage and allocating 100s of megabytes of arrays that would be never used is not something I'd do lightly.
context.classes.clear(); | ||
context.weights.clear(); | ||
for (auto cls : context.labels) { | ||
context.classes.push_back(cls); |
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.
The above comment is because this looked really familiar to 230-231.
GetLabelBoundingBoxes(boxes, ctx.blobs.to_static<static_ndim>(), -1); | ||
int box_idx = PickBox(boxes, ctx.sample_idx); | ||
if (box_idx >= 0) { | ||
ctx.SelectBox(box_idx); |
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 you already did the reinterpret why not:
ctx.SelectBox(box_idx); | |
ctx.SelectBox(boxes[box_idx]); |
assert(threshold_.get().shape[sample_idx] == TensorShape<1>{ ndim }); | ||
for (int i = 0; i < ndim; i++) | ||
threshold[i] = thresh[i]; | ||
end = std::remove_if(beg, end, [threshold](auto box) { |
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.
Any difference if the auto box
was a reference instead of pass by value?
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.
It probably should be.
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 focused mostly on the test coverage (and didn't dig that deep into generating the test data and results in python), so mostly suggestions for some more testing that could be done.
|
||
def test_num_output(): | ||
"""Test that a proper number of outputs is produced, depending on arguments""" | ||
inp = fn.external_source(data, batch=False) |
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 was looking where the data comes from, and it surprised me below. Could you maybe move it to the top?
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.
Sure.
threshold_opt = [None, 3, list(range(1, 1+ndim)), fn.random.uniform(range=(1,5), shape=[ndim], dtype=dali.types.INT32, seed=13231)] | ||
threshold = np.random.choice(threshold_opt); | ||
k_largest_opt = [None, 1, 2, 5] | ||
k_largest = np.random.choice(k_largest_opt); |
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 not sure if having threshold and k_largest as randomly tested is a good decision, probably it would be safer to make sure that they are tested with both unspecified, one of them specified and both specified.
To not have have cartesian product of all possibilities, you could probably test that for a praticular setting of classes etc.
|
||
return label | ||
|
||
def batch_generator(batch_size, ndim, dtype): |
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.
Would it make sense to test with data that would give deterministic boxes as a result, for example have classes with 1 box, and set the weight for one of them to 1 and rest to 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.
That's requesting 100s extra lines of tests for something that's pretty well covered by existing tests.
from test_utils import check_batch | ||
from test_utils import np_type_to_dali | ||
from nose.tools import raises, assert_raises, nottest | ||
|
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 would maybe consider adding negative tests, stuff like not matching number of weights and classes passed should raise, etc. and if the ignore_classes is specified other class-related arguments should cause an error.
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 can try.
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
CI MESSAGE: [2083639]: BUILD STARTED |
CI MESSAGE: [2083639]: BUILD PASSED |
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
JIRA TASK: DALI-1817