Fix random cone test

Fixes #870 (closed).

In a recent MR, the function random_direction_in_cone that generates a random unit vector within a given cone was modified. It no longer takes both the ID of a BH and a gas particle as inputs, but only the former. However, this means that the testRandomCone.h test was less random than we wanted, and was thus failing.

I have modified that test in a very simple way. It still does exactly the same thing, but we now generate a random id id_random and pass id_BH + id_random instead of id_BH to the random_direction_in_cone function. The test again passes with this change.

requested review from @matthieu

mentioned in issue #870 (closed)

If the function is less random than before shouldn't we fix the function rather than the test?

changed the description

added bug label

I'm not sure, it is less random simply on account of the fact that it no longer takes in two ids, but one. In terms of how it is implemented in the code: a new random direction is drawn every time before a kick happens, so it should be random enough for the model purposes.

Ok, but that is not how you call the function in the code. So the test is not testing the real life use case.

That's true. However I would then need to simplify the test. This is the part in test_cone where things go wrong:

  for (int k = 0; k < N_test; ++k) {
      
    /* Generate a random unit vector within a cone around unit_vector  */
    random_direction_in_cone(id_bh, ti_current, type, opening_angle,
                             unit_vector, rand_vector);

    /* Check that this vector is actually within the cone we want  */
    const double cos_rand_unit = rand_vector[0] * unit_vector[0] +
                                 rand_vector[1] * unit_vector[1] +
                                 rand_vector[2] * unit_vector[2];
    if (cos_rand_unit < 0.99999 * cos_theta_max) {
      printf("Cos_opening_angle is: %f, Random cos is: %f\n", cos_theta_max,
             cos_rand_unit);
      error("Generated random unit vector is outside cone.");
    }

    /* Add the unit vector to the probability density function array. The solid
     * angle subtended by some angle theta grows as (1-cos(theta)). Furthermore,
     * we are limited to the spherical cap defined by the angles [0, theta_max].
     * Therefore the variable which we expect to be uniformly distributed is (1
     * - cos(theta)) / (1 - cos(theta_max)). */
    double uniform_variable = (1. - cos_rand_unit) / (1 - cos_theta_max);
    for (int j = 0; j < N_bins; ++j) {
      if ((uniform_variable > (double)j / (double)N_bins) &&
          (uniform_variable < (double)(j + 1) / (double)N_bins)) {
        binned_cosines[j] = binned_cosines[j] + 1. / (double)N_test;
      }
    }
  }

For every value k in this loop, we used to generate a new gas id, which would be fed to random_direction_in_cone. However, as things stand now, random_direction_in_cone above always seems to generate exactly the same vector (in total, N_test times). I am not entirely sure why. It may be because bh_id is used every time in the loop, and it never changes.

Edit: So I could get rid of the loop (but then the test is no longer testing the uniformity of this function), or add a randomly generated id somewhere else in this process, but I am not sure I can do that in a way that matches exactly what is done in the code.

Shouldn't you then not look over k but check that many calls with different BH_ids do lead to a uniform distribution?

added 1 commit

• 3fb98a8d - Update file testRandomCone.c

Compare with previous version

added 1 commit

• 5b7227ee - Formatting script

Compare with previous version

Indeed that's one way of doing it. I have now implemented that and changed things a bit. test_cone now simply draws one random unit vector (within a cone of specified angle opening_angle around unit_vector, both of which are inputs for the function). It also checks whether the drawn vector is within that cone (simply by calculating the cosine of the angle).

I then do three separate tests using this function:

1. Whether this works for different opening angles
2. A uniformity test (for a single opening angle)
3. A check where the unit vectors (that define the cone directions) are not drawn randomly but set along a grid with possibly problematic configurations like (0, 0, 1), etc., where things may diverge.

added 1 commit

• da172025 - Report the random seed used at the start of a testRandomCone unit test

Compare with previous version

That looks good! I have just added a bit of i/o at the start to report what seed is used. This is useful if it crashes as we can then re-run the exact setup.

approved this merge request

merged

mentioned in commit d5480423