Fix issue when applying crf layer on batch_size>1

SamuelJoutard commented 5 years ago

This issue has not been reported, yet if you do simple test with this layer with batch larger than 1, it will combine the information across batch elements. This branch fix this.

Status

READY

Description

cf above.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[x] My code follows the code style of this project.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

Todos

[ ] Tests
[ ] Documentation

Impacted Areas in Application

List general components of the application that this PR will affect: *

ericspod commented 5 years ago

Is there anyway to create a simple test case for this change?

SamuelJoutard commented 5 years ago

Yes there is. I can write the test in TF if needed. The basic idea is to consider for instance: desc = np.zeros((2, 5, 5, 5, 1)) desc[0] = 1 feat = np.random.rand((2, 5, 5, 5, 5)) as inputs. desc corresponds to data_vector and feat to position_vectors. Here, one element of the batch has only 0 as data_vectors and the other has only ones (position vectors are random). If you apply the functions permutohedral_prepare and permutohedral_compute the information should not be mixed across batches elements so a way to observe the issue I am pointing is to first check the output of: permutohedral_compute(permutohedral_prepare()) (adjusting the arguments). And see that the output corresponding to the first batch element does not only contains zeros. Then a way to verify the proposed change is to run the crf_as_rnn test on the updated version of the code.

wyli commented 5 years ago

Thanks @SamuelJoutard, would be great to add the test case to https://github.com/NifTK/NiftyNet/blob/dev/tests/crf_test.py#L10

SamuelJoutard commented 5 years ago

Hi, I updated the test_crf.py file. It now contains an extra test to check the issue I was mentioning. Those modifications passed all tests on my laptop. I hope it will help!

NifTK / NiftyNet