Fix layout propagation in Erase #2133
Conversation
Fixes both Erase CPU and GPU. Adjust compare_pipelines and check_batch to allow layout checking, the former can accept tuple of layouts or single value, the check is optional. Adjust Erase Python tests. Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
|
!build |
|
CI MESSAGE: [1481021]: BUILD STARTED |
| assert batch.layout() == expected_layout, \ | ||
| 'Unexpected layout, expected "{}", got "{}".'.format(expected_layout, batch.layout()) | ||
|
|
||
| if compare_layouts and \ |
There was a problem hiding this comment.
isn't the first block of code enough? Why are you checking it again?
There was a problem hiding this comment.
It's to compare between two batches (as opposed to providing the layout as argument to this function and checking if (where it can be check) we have a match with expected layout.
| auto output = view<T, Dims>(ws.template OutputRef<GPUBackend>(0)); | ||
| const auto &input_ref = ws.template InputRef<GPUBackend>(0); | ||
| auto &output_ref = ws.template OutputRef<GPUBackend>(0); | ||
| output_ref.SetLayout(input_ref.GetLayout()); |
There was a problem hiding this comment.
I'd rather use Operator's InputLayout function - I know that we don't have a default layout here, but it's proliferating the pattern which is not correct for the case with default input layout present.
There was a problem hiding this comment.
Hmm, the implementations are using the Operator class directly, but use the OperatorImpl as base and don't have access to this function. I will add this to the follow-up task, maybe we can do something better with it.
| @@ -138,7 +181,12 @@ def compare_pipelines(pipe1, pipe2, batch_size, N_iterations, eps = 1e-07): | |||
| for i in range(len(out1)): | |||
| out1_data = out1[i].as_cpu() if isinstance(out1[i][0], dali.backend_impl.TensorGPU) else out1[i] | |||
| out2_data = out2[i].as_cpu() if isinstance(out2[i][0], dali.backend_impl.TensorGPU) else out2[i] | |||
| check_batch(out1_data, out2_data, batch_size, eps) | |||
| if isinstance(expected_layout, tuple): | |||
There was a problem hiding this comment.
I would say it's a test util, you need a iterable, you can extend it to your liking, but I don't see why we need to support 3 different possibilities here (as none is currently used).
|
CI MESSAGE: [1481021]: BUILD FAILED |
|
CI MESSAGE: [1481021]: BUILD PASSED |
Signed-off-by: Krzysztof Lecki klecki@nvidia.com
Why we need this PR?
Fix layout propagation in Erase
What happened in this PR?
What solution was applied:
SetLayout(GetLayout())
Fixes both Erase CPU and GPU.
Adjust compare_pipelines and check_batch to allow layout checking,
the former can accept tuple of layouts or single value,
the check is optional.
Adjust Erase Python tests.
Affected modules and functionalities:
Erase, Python test utils.
Key points relevant for the review:
NA
Validation and testing:
Test adjusted
Documentation (including examples):
NA
JIRA TASK: [DALI-1515]