-
-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to return indexes in conditional abundance matching #913
Changes from 10 commits
e98bb54
14dbca3
dad0dfe
f038681
4792045
9cd1b0e
c1e0e48
405bee4
6e60ca2
c6d1896
f05be1c
15251e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
from .bin_free_cam_kernel import cython_bin_free_cam_kernel | ||
from .bin_free_cam_kernel import cython_bin_free_cam_kernel, get_value_at_rank |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import numpy as np | |
cimport cython | ||
from ....utils import unsorting_indices | ||
|
||
__all__ = ('cython_bin_free_cam_kernel', ) | ||
__all__ = ('cython_bin_free_cam_kernel', 'get_value_at_rank') | ||
|
||
|
||
cdef double random_uniform(): | ||
|
@@ -110,11 +110,35 @@ def setup_initial_indices(int iy, int nwin, int npts): | |
return iy, init_iy_low, init_iy_high | ||
|
||
|
||
@cython.boundscheck(False) | ||
@cython.nonecheck(False) | ||
@cython.wraparound(False) | ||
cdef int _find_index(int[:] arr, int val): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to make sure I understand what you are suggesting: I think that this has roughly the same time complexity - when we update the correspondence indicies (to shift them along) we will need to update this array too and that would be roughly But I could well have missed something... I definitely don't fully understand all the internals. (index There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may be right, this might not be worth the trouble. After cleaning the cython of the remaining python interactions as discussed elsewhere, could you just do a simple timing test comparing your implementation against what is currently in master? If runtimes are comparable, then it means that further optimization is not worth spending time on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the timing test, just make sure to use at least ~1e6 points There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the new code
In master
So returning indexes is ~20% slower, probably due to that extra linear search over the window. I used 1e7 points in both x and y and a 10001 sized window. But no change in the returning values case. |
||
for i in range(len(arr)): | ||
if arr[i] == val: | ||
return i | ||
return -1 | ||
|
||
|
||
@cython.boundscheck(False) | ||
@cython.nonecheck(False) | ||
@cython.wraparound(False) | ||
def get_value_at_rank(double[:] sorted_values, int rank1, int nwin, int add_subgrid_noise): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, quick implementation comment about this In general, it's good to get in the habit of always running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I But maybe a better way would be to have an internal Running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sweet, thanks for the comments! |
||
if add_subgrid_noise == 0: | ||
return sorted_values[rank1] | ||
else: | ||
low_rank = max(rank1 - 1, 0) | ||
high_rank = min(rank1 + 1, nwin - 1) | ||
low_cdf = sorted_values[low_rank] | ||
high_cdf = sorted_values[high_rank] | ||
return low_cdf + (high_cdf-low_cdf)*random_uniform() | ||
|
||
|
||
@cython.boundscheck(False) | ||
@cython.nonecheck(False) | ||
@cython.wraparound(False) | ||
def cython_bin_free_cam_kernel(double[:] y1, double[:] y2, int[:] i2_match, int nwin, | ||
int add_subgrid_noise=0): | ||
int add_subgrid_noise, int return_indexes): | ||
""" Kernel underlying the bin-free implementation of conditional abundance matching. | ||
For the i^th element of y1, we define a window of length `nwin` surrounding the | ||
point y1[i], and another window surrounding y2[i2_match[i]]. We calculate the | ||
|
@@ -159,7 +183,9 @@ def cython_bin_free_cam_kernel(double[:] y1, double[:] y2, int[:] i2_match, int | |
cdef int idx_in1, idx_out1, idx_in2, idx_out2 | ||
cdef double value_in1, value_out1, value_in2, value_out2 | ||
|
||
cdef double[:] y1_new = np.zeros(npts1, dtype='f8') - 1 | ||
cdef int[:] y1_new_indexes = np.zeros(npts1, dtype='i4') - 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to define these in if statements:
but that wasn't allowed? Defining both is a bit unnecessary, but I couldn't find a nice way around it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I don't know a way around this either. This seems fine. |
||
cdef double[:] y1_new_values = np.zeros(npts1, dtype='f8') - 1 | ||
|
||
cdef int rank1, rank2 | ||
|
||
# Set up initial window arrays for y1 | ||
|
@@ -201,58 +227,47 @@ def cython_bin_free_cam_kernel(double[:] y1, double[:] y2, int[:] i2_match, int | |
for iy1 in range(nhalfwin, npts1-nhalfwin): | ||
|
||
rank1 = correspondence_indx1[nhalfwin] | ||
iy2_match = i2_match[iy1] | ||
|
||
# Stop updating the second window once we reach npts2-nwin/2 | ||
if iy2_match > iy2_max: | ||
iy2_match = iy2_max | ||
# Where to center the second window (making sure we don't slide off the end) | ||
iy2_match = min(i2_match[iy1], iy2_max) | ||
|
||
if iy2 > iy2_max: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure that this shouldn't be possible and this set of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed this is a little easier to read, but
After looking at the Also watch out for this on lines 130 & 131 inside the definition of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, as these are http://cython.readthedocs.io/en/latest/src/userguide/numpy_tutorial.html suggests that cython has a fast min/max for ints (as you saw in the |
||
iy2 = iy2_max | ||
else: | ||
# Continue to slide the window along the second array | ||
# until we find the matching point, updating the window with each iteration | ||
while iy2 < iy2_match: | ||
# Continue to slide the window along the second array | ||
# until we find the matching point, updating the window with each iteration | ||
while iy2 < iy2_match: | ||
|
||
# Find the value coming in and the value coming out | ||
value_in2 = y2[iy2 + nhalfwin + 1] | ||
idx_out2 = correspondence_indx2[nwin-1] | ||
value_out2 = sorted_cdf_values2[idx_out2] | ||
# Find the value coming in and the value coming out | ||
value_in2 = y2[iy2 + nhalfwin + 1] | ||
idx_out2 = correspondence_indx2[nwin-1] | ||
value_out2 = sorted_cdf_values2[idx_out2] | ||
|
||
# Find the position where we will insert the new point into the second window | ||
idx_in2 = _bisect_left_kernel(sorted_cdf_values2, value_in2) | ||
if value_in2 > value_out2: | ||
idx_in2 -= 1 | ||
# Find the position where we will insert the new point into the second window | ||
idx_in2 = _bisect_left_kernel(sorted_cdf_values2, value_in2) | ||
if value_in2 > value_out2: | ||
idx_in2 -= 1 | ||
|
||
# Update the correspondence array | ||
_insert_first_pop_last_kernel(&correspondence_indx2[0], idx_in2, nwin) | ||
for j in range(1, nwin): | ||
idx2 = correspondence_indx2[j] | ||
correspondence_indx2[j] += _correspondence_indices_shift( | ||
idx_in2, idx_out2, idx2) | ||
# Update the correspondence array | ||
_insert_first_pop_last_kernel(&correspondence_indx2[0], idx_in2, nwin) | ||
for j in range(1, nwin): | ||
idx2 = correspondence_indx2[j] | ||
correspondence_indx2[j] += _correspondence_indices_shift( | ||
idx_in2, idx_out2, idx2) | ||
|
||
# Update the CDF window | ||
_insert_pop_kernel(&sorted_cdf_values2[0], idx_in2, idx_out2, value_in2) | ||
# Update the CDF window | ||
_insert_pop_kernel(&sorted_cdf_values2[0], idx_in2, idx_out2, value_in2) | ||
|
||
iy2 += 1 | ||
iy2 += 1 | ||
|
||
# The array sorted_cdf_values2 is now centered on the correct point of y2 | ||
# We have already calculated the rank-order of the point iy1, rank1 | ||
# So we either directly map sorted_cdf_values2[rank1] to ynew, | ||
# or alternatively we randomly draw a value between | ||
# sorted_cdf_values2[rank1-1] and sorted_cdf_values2[rank1+1] | ||
if add_subgrid_noise == 0: | ||
y1_new[iy1] = sorted_cdf_values2[rank1] | ||
if return_indexes == 1: | ||
index = _find_index(correspondence_indx2, rank1) | ||
if index == -1: | ||
raise Exception("Index {} not found in correspondence_indx2".format(rank1)) | ||
y1_new_indexes[iy1] = iy2 + nhalfwin - index | ||
else: | ||
low_rank = rank1 - 1 | ||
high_rank = rank1 + 1 | ||
if low_rank < 0: | ||
low_rank = 0 | ||
elif high_rank >= nwin: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, no problem at all to leave as is - mostly I was pointing this out for pedagogical reasons: since this is one of your first cython function contributions I thought it might be helpful to be explicitly aware of all python-layer interactions. But |
||
high_rank = nwin - 1 | ||
low_cdf = sorted_cdf_values2[low_rank] | ||
high_cdf = sorted_cdf_values2[high_rank] | ||
y1_new[iy1] = low_cdf + (high_cdf-low_cdf)*random_uniform() | ||
y1_new_values[iy1] = get_value_at_rank(sorted_cdf_values2, rank1, nwin, add_subgrid_noise) | ||
|
||
# Move on to the next value in y1 | ||
|
||
|
@@ -276,4 +291,6 @@ def cython_bin_free_cam_kernel(double[:] y1, double[:] y2, int[:] i2_match, int | |
# Update the CDF window | ||
_insert_pop_kernel(&sorted_cdf_values1[0], idx_in1, idx_out1, value_in1) | ||
|
||
return y1_new | ||
if return_indexes: | ||
return y1_new_indexes | ||
return y1_new_values |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,13 +2,15 @@ | |
""" | ||
import numpy as np | ||
from astropy.utils.misc import NumpyRNGContext | ||
import pytest | ||
from ..bin_free_cam import conditional_abunmatch | ||
from ....utils.conditional_percentile import cython_sliding_rank, rank_order_function | ||
from .naive_python_cam import pure_python_rank_matching | ||
from ....utils import unsorting_indices | ||
|
||
|
||
fixed_seed = 43 | ||
fixed_seed2 = 44 | ||
|
||
|
||
def test1(): | ||
|
@@ -384,11 +386,13 @@ def test_subgrid_noise1(): | |
result2 = conditional_abunmatch(x, y, x2, y2, nwin1, add_subgrid_noise=True) | ||
assert np.allclose(result, result2, atol=0.1) | ||
assert not np.allclose(result, result2, atol=0.02) | ||
assert np.all(result - result2 != 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be enough to assert that we are adding noise to the leftmost/rightmost edges. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this actually ensure that noise was added to the edges? Or just that noise was added somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it ensures that noise was added to every point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you're right, upon second look I see that now. |
||
|
||
nwin2 = 1001 | ||
result = conditional_abunmatch(x, y, x2, y2, nwin2, add_subgrid_noise=False) | ||
result2 = conditional_abunmatch(x, y, x2, y2, nwin2, add_subgrid_noise=True) | ||
assert np.allclose(result, result2, atol=0.02) | ||
assert np.all(result - result2 != 0) | ||
|
||
|
||
def test_initial_sorting1(): | ||
|
@@ -503,3 +507,40 @@ def test_initial_sorting4(): | |
assume_x_is_sorted=True, assume_x2_is_sorted=True, | ||
add_subgrid_noise=False) | ||
assert np.allclose(result, result4[unsorting_indices(idx_x_sorted)]) | ||
|
||
def test_no_subgrid_noise_with_return_indexes(): | ||
x, y = np.arange(5), np.arange(5) | ||
x2, y2 = np.arange(10), np.arange(10) | ||
nwin = 3 | ||
with pytest.raises(ValueError) as err: | ||
conditional_abunmatch(x, y, x2, y2, nwin, add_subgrid_noise=True, return_indexes=True) | ||
assert str(err.value) == "Can't add subgrid noise when returning indexes" | ||
|
||
|
||
def test_return_indexes(): | ||
n1, n2 = int(1e2), int(1e2) | ||
|
||
with NumpyRNGContext(fixed_seed): | ||
x = np.random.uniform(0, 10, n1) | ||
y = np.random.uniform(0, 1, n1) | ||
|
||
with NumpyRNGContext(fixed_seed2): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I used the same seed here as above tests were passing when they shouldn't (before I had correctly reordered/reindexed the indexes). I think that was because we were in the special case of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, yes, good catch, sometimes I have to use different seeds for such purposes as well. |
||
x2 = np.random.uniform(0, 10, n2) | ||
y2 = np.random.uniform(-4, -3, n2) | ||
|
||
nwin = 9 | ||
for sorted_x in [False, True]: | ||
for sorted_x2 in [False, True]: | ||
x_, y_, x2_, y2_ = x, y, x2, y2 | ||
if sorted_x: | ||
x_, y_ = np.sort(x_), np.sort(y_) | ||
if sorted_x2: | ||
x2_, y2_ = np.sort(x2_), np.sort(y2_) | ||
|
||
|
||
values = conditional_abunmatch(x_, y_, x2_, y2_, nwin, add_subgrid_noise=False, | ||
assume_x_is_sorted=sorted_x, assume_x2_is_sorted=sorted_x2, return_indexes=False) | ||
indexes = conditional_abunmatch(x_, y_, x2_, y2_, nwin, add_subgrid_noise=False, | ||
assume_x_is_sorted=sorted_x, assume_x2_is_sorted=sorted_x2, return_indexes=True) | ||
|
||
assert np.all(y2_[indexes] == values), "{}, {}".format(sorted_x, sorted_x2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the best api is - another option would be to have two functions:
conditional_abunmatch
andconditional_abunmatch_indexes
. I don't know how this is done elsewhere in halotools/how you would like to do it going forward. Your call :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only time I prefer creating an entirely new function is in cases where adding in some new feature considerably slows performance; if performance is mission-critical to the original function, I just write a new one. The additional algebraic operations added here are minor enough that I think what you've done here is preferable.