Update test_vesseltree.py

[jgroehl]

Disable test that throws errors

Fixes #297

[jgroehl]

This test can sometimes fail, because of the random nature of the variations.
Radius variation is triggered upon the creation of each vessel, but the effect might be smaller than a pixel.
This test would require a fixed random seed, but even that does not guarantee that it works across machines and operating systems. As such I would recommend not to run this test at all!

[leoyala]

I think that we should not remove the test, more details in the comment.

[leoyala]

I think that we should not remove the test but rather make it stable. For example by setting a seed, and disclosing in the documentation that setting vesseltree_settings[Tags.STRUCTURE_RADIUS_VARIATION_FACTOR] = 1 might involve small variations due to rounding errors.

[jgroehl]

A seed will not make the execution stable across OSs and machines, as the underlying PRNG might yield different number sequences. And I am not sure what you mean by the rounding errors. This sets the radius variation to +/- one pixel, but it is equally sampled between -1 and 1 -> depending on the discretization of the simulation grid, this might not result in a whole-pixel change of the vessel boundary.

[leoyala]

I agree that setting a seed would not fix it actually.
And I understand what you mean now from this:
https://github.com/IMSY-DKFZ/simpa/blob/develop/simpa/utils/libraries/structure_library/VesselStructure.py#L105
However, I would have imagined that when you set vesseltree_settings[Tags.STRUCTURE_RADIUS_VARIATION_FACTOR] = 1 that this would translate to no variation, since it is a "factor" after all. And this is not the case because that line of code assumes that it is always different from 1. I think a better approach would be to set something like:

if radius_variation == 1:
    radius_array = radius
else: 
    radius_array.append(np.random.uniform(-1, 1) * radius_variation + radius)

[jgroehl]

At that point, the easier fix would probably be to rename the variable, such that it instead reads:

Tags.STRUCTURE_MAX_VESSEL_RADIUS_VARIATION

?

[leoyala]

I don't think that would be a fix. Because we would still have a variable that most of the times behaves as a multiplicative factor, except when you set it to 1. And I think that the fix with adding one "if" statement is quite straightforward. 

[jgroehl]

Hey Leo, I am not quite sure why you assume it is multiplicative, though! I do not think it is.
The implementation is:

radius_array.append(np.random.uniform(-1, 1) * radius_variation + radius)

So, it is an additive component to the radius that can affect the radius by a maximum of +/- 1 * the radius_variation. new_radius = radius + variation, where the extent of the variation is uniformly randomly determined. radius_variation is in the same units as the radius, which within the structures should be in pixel units.

Unless I am missing something, it is not affecting the radius multiplicative.
If that is true, then the proposed fixed at variation==1 does not make sense, as the variation would slowly increase from 0 to 0.999, then drop to 0 at 1 and then further gradually increase.

What we could do is rewrite the test, to test that the radius remains unchanged if we set radius_variation to 0!
This would call the same lines of code for the test coverage, but not test the same functionality.

[leoyala]

I see what you mean, I must have overlooked that. In this case, renaming the variable and checking the behavior when it is set to 0 should do the trick, as you suggested. But you should also update the docstring in the Tags class and check that other parts of the code do not use it as a factor but as intended.

[frisograce]

Hi, I've had a look over my code and I think I may have found a better fix.

My code has an error as I forgot to account for the "radius_margin" during the acquisition of the enclosed indices. This has meant that in order to be able to trigger that there was indeed an increase in radius, the radius change required had to be greater than this radius margin significantly reducing the likelihood for it would occur. Having now seen how this works, I believe a more appropriate fix would be to enclose this margin so that any variation will trigger the checks and pass the tests.

Indeed this is still a probabilistic approach, as it needs some radius variation to occur. However given that there are 10 instances along the length where the radius can vary the probability of no variation along any of those elements becomes vanishingly small.

My proposed fix would look more like this:

def test_radius_variation_factor(self):
    """
    Test radius variation factor
    Let there be no bifurcation or curvature, and see if the radius changes
    :return: Assertion for radius variation
    """
    self.vesseltree_settings[Tags.STRUCTURE_RADIUS_MM] = 2.5
    self.vesseltree_settings[Tags.STRUCTURE_RADIUS_VARIATION_FACTOR] = 1
    ts = VesselStructure(self.global_settings, self.vesseltree_settings)

    vessel_centre = 5
    edge_of_vessel = vessel_centre + self.vesseltree_settings[Tags.STRUCTURE_RADIUS_MM]
    has_reduced = np.min(ts.geometrical_volume[int(edge_of_vessel-0.5), :, vessel_centre]) != 1
    has_increased = np.max(ts.geometrical_volume[int(edge_of_vessel+0.5), :, vessel_centre]) != 0

    assert has_reduced or has_increased

If it's agreed that we should make it deterministic then I'm happy to amend this to a model where we only consider if the radius hasn't change, but i think a more effective test should determine if the radius can vary.

[jgroehl]

That sounds reasonable to me!
I also thought that the radius would only change on bifurcation - but this is not true! It's each step, with a unit vector step size :)

Could we perhaps test how often the test would fail now and if it can happen sometimes (let's say >0.1%), call the function in a loop N times, where the test is successful as soon as a run succeeds?

[frisograce]

I've run this 5 times, and overall only twice did an individual element actually fail. To me, this indicates that it will work on a likelihood of each element failing being approximately <1/10 and therefore over 10 iterations the chance is minuscule (1e-10).

[leoyala]

Looks ok to me @frisograce, but you should remove the previous test test_radius_variation_factor, it is currently only commented in the code, but if we do not need it anymore, it should just be removed.

[frisograce]

Yes definitely, thanks!