Mismatch of docs and definition of postTools.sample_prob

[User-zwj]

1. Line 146: indices is not defined if sort is False. My guess is that 'if descending' might be within 'if sort'

2. Line 139: it seems to me that 'if descending' should return sample by the descending order of the probability. But when I checked the code of the function 'sort_by_rho',
   Line 59: indices = np.argsort(P_samples)[::-1][0:nnz]
   [::-1] will make it return the indices in descending order. This means that 'indices' in Line 128 is the descending index, so I think there is no need of Line 146. Then Line 140-145 will also make those terms sort by ascending order.

[mathematicalmichael]

Try getting rid of the line and seeing if all the tests still pass?

[User-zwj]

I reread the comment in the function, here is my idea.

1. I think there is no need to put 'sort' variable in the function of sample_prob. Because we always need to sort the samples in order to get highest/lowest probability samples.
2. 'descending' is like an indicator to show we want highest probability samples or lowest probability samples. descending = True, we will get highest probability samples; descending = False, we will get lowest probability samples.

I can get the test pass, but I am not sure if I missed something or I misunderstood the point of this function.

[mathematicalmichael]

looking at it more closely now... yeah, I see what you mean. If sort=False ... The function tries to return a variable that is undefined.

Looking at the docstring, I see that indices is actually supposed to be passed to the function.

My guess, and @eecsu can weigh in, is that this was simply omitted accidentally, and that indices=None is supposed to be in the function definition, with a line at the top of the method along the lines of:

if indices is None:
    indices = np.arange(sample_set.check_num())

Then everything seems to fall into place and makes more sense.
If descending=False, and sort=False, the indices return in the order they appear. Since this exact combination is functionally useless, returning an unsorted set of samples, it does not surprise me that no test was written for this exact case.

It makes sense to make the change I mentioned above, so the documentation aligns with the definition. But thinking more about how this function should work, I think we can also do this:

• remove indices from documentation. Perhaps it was left over from a draft version.
• remove sort=True, and just always sort the samples. Right now this function will error out if sort=False because of the reason @User-zwj mentioned... indices is never defined.
• remove line 127 and de-indent that section.

This should also allow the tests to pass. What is currently Line 146 should stay in.

[mathematicalmichael]

Alternative: add functionality by allowing sorting on a subset of indices, but in that case we'll need to trim the sample set before passing to sort_by_rho and return that as sample_set_out. This may have been part of the intention for defining indices in the docstring. That change would require an else statement after if sort on 127 as well.