Test + docs: layer_utils.py about deepreg HOT 13 CLOSED

@YipengHu The whole point of unit testing is asserting that mathematically the function does what we expect it to do, consistently. Unit testing the code really helps down the line when things break / start working in funky ways that are unexpected - because the repo is quite large and has lots of moving parts, it will be hard to diagnose what is not working because of some unexpected behaviour in the code or because of changes in the maths...

Understandably with random transforms this is a bit hard but @zacbaum do you think it would be more feasible to test the function if it had a seed that could be set for tests (for a couple of cases) and generate the output transforms separately?

Maybe more importantly, before we progress with unit testing, if the current documentation standards of the code are not sufficient, we may have to do a big documentation run so that unit testing is facilitated for people that haven't originally developed the code.

from deepreg.

@zacbaum please have a look at #101
I added the tests while adding comments and doc string, please do go through all the funcs to see if you can understand it

from deepreg.

btw what do you mean a pytest style?

i followed https://docs.python.org/3/library/unittest.html#basic-example where we use the function self.assertEqual ? well it's equivalent to the assert anyway

I think you've been using the unittest package, which is an extra dependency - whereas pytest doesn't require any imports - some documentation here.

from deepreg.

I think this should be ready for PR, build is passing lint (although not manifesting in this thread, can be observed here: link to travis)

@YipengHu @zacbaum

from deepreg.

@mathpluscode - It's not completely obvious to me what happens / why things are done in pyramid_combination, random_transform_generator and warp_grid.

Could you let me know what kinds of things need to be tested other than input/output validity (i.e. types and sizes)?

What are these functions used for within the rest of the repo?

If you let me know, I will update the docstrings for those functions along with the creation of these unit tests.

from deepreg.

@zacbaum
pyramid_combination the function description is in, primarily for interpolating intensity on voxels (the resampler);
random_transform_generator generate random affine transformation by oscillating (normalised to -1 and 1) corner points with provided scale parameter. this is for data augmentation;
warp_grid warp the grid (3d coordinates of all voxels) using an affine transformation parameterised by theta - this is probably not best function name - should be warp_grid_affine or something. used in resampling images with given affine transform.

from deepreg.

@zacbaum
pyramid_combination the function description is in, primarily for interpolating intensity on voxels (the resampler);
random_transform_generator generate random affine transformation by oscillating (normalised to -1 and 1) corner points with provided scale parameter. this is for data augmentation;
warp_grid warp the grid (3d coordinates of all voxels) using an affine transformation parameterised by theta - this is probably not best function name - should be warp_grid_affine or something. used in resampling images with given affine transform.

So, beyond input/output validity - what should be tested for these? If the transforms generated are random, it would be difficult to test much there other than that they are well-formed (invertible, etc), no?

I'm not familiar with some of the functions used and without going through the remaining three functions line-by-line to see the outputs of the intermediate these methods; the underlying math is not completely obvious to me. And yes, the documentation is there, but all that's provided for warp_grid is 'perform transformation on the grid' - which is not really helpful...

For the most part, any inline comments only provide expected dimensions but do not describe what's actually occurring. Without knowing this, it is difficult to determine how to best test these functions.

from deepreg.

@zacbaum

Are we talking about unit testing here, right? You don't have to know exactly what it is doing to unit test a function - sure it could help. The invertibility etc are more like a regression test. I would "assume" the current one is correct and generate some test data (small in size please) and, in this way, the unit tests can at least make sure the future changes won't break this function (to certain level). If in the end, we find out this is not doing what it suppose to do, that's another issue. :)

Here you go:
pyramid_combination is doing this: https://en.wikipedia.org/wiki/Trilinear_interpolation#Method
'perform transformation on the grid' is literally grid_coordinates * A = warped_grid_coordinates. the rest are changing shapes etc.

Let me know if there is anything specific that is confusing.

I would say adding a

from deepreg.

sorry for the delay, i will come back to this tmr to provide more details on the descriptions of these methods and also how i think we can test them in the proper way

from deepreg.

sorry for the delay, i will come back to this tmr to provide more details on the descriptions of these methods and also how i think we can test them in the proper way

@mathpluscode - please let me know when this is done :)

from deepreg.

@mathpluscode, thank you for adding these comments, this will be really useful going forward.

In the future, if someone is actively working on something (and a branch already exists for that work), it would be appreciated if the work can be integrated into that existing branch so we aren't doing things redundantly. Please make sure to check with the person assigned to the ticket before trying to finish their job for them :)

Additionally, @NMontanaBrown has asked that the unit testing be done with pytest (as noted in the ticket description), and the testing you've created will need to be refactored so I'm going to reopen this ticket, merge your commits into the branch I've been working on this is, and make those refactoring changes.

from deepreg.

@zacbaum sure, sorry i didnt pay attention that you had a branch earlier

btw what do you mean a pytest style?

i followed https://docs.python.org/3/library/unittest.html#basic-example where we use the function self.assertEqual ? well it's equivalent to the assert anyway

from deepreg.

if you expect failure maybe use @unittest.expectedFailure` ? and actually maybe it's better to split the cases like 1d 2d 3d etc...

from deepreg.