Updates for find pairs - #68
patricoferris wants to merge 1 commit into
Conversation
d1e36d8 to
889b604
Compare
889b604 to
9fc5c08
Compare
|
|
||
| # Methodology 6.5.5: S should be 10 times the size of K | ||
| required = k_set.shape[0] * 10 | ||
| s_dist_hard = np.ascontiguousarray(s_set[hard_match_columns].to_numpy()).astype( |
There was a problem hiding this comment.
Comment above is wrong (float32) and we can do dtype=np.int32 on the first call and maybe save a conversion.
| s_dist_hard = np.ascontiguousarray(s_set[hard_match_columns].to_numpy()).astype( | ||
| np.int32 | ||
| ) | ||
| k_dist_hard = np.ascontiguousarray(k_subset[hard_match_columns].to_numpy()).astype( |
robinmessage
left a comment
robinmessage
left a comment
There was a problem hiding this comment.
These changes make sense to me. Without profiling, I can't see any particular performance issues this would introduce compared to the existing code.
I have left a few minor comments mostly for neatness. I also have a more substantive question about methodology and how what is implemented relates to 6.5.4.
| s_subset_mask = make_s_subset_mask(s_dist_thresholded, k_dist_thresholded, s_dist_hard, k_dist_hard, required) | ||
| s_subset_mask_true = np.zeros(s_set.shape[0], dtype=np.bool_) | ||
|
|
||
| # Randomly select 100 potential pixels per K, these pixels are similar |
There was a problem hiding this comment.
Is this to match the methodology step 6.5.4? I don't see anything that then limits to |K| * 10 but maybe I've misunderstood.
Also, that part of the methodology mentions choosing from R "alternating between randomly or stratified by land cover class" - I'm not sure what "stratified by land cover class" means, but we only seem to do randomly.
robinmessage
left a comment
robinmessage
left a comment
There was a problem hiding this comment.
Sorry, I forgot I wanted to comment on this bit: I think you need to include some more rows in matchless.
| potentials = np.invert(no_potentials) | ||
|
|
||
| logging.info("Finished preparing s_subset. shape: %s", s_subset.shape) | ||
| k_subset = k_subset[potentials] |
There was a problem hiding this comment.
I think you need to k_subset[no_potentials] as something like k_not_matchable...
| @@ -151,35 +191,36 @@ def find_match_iteration( | |||
| k_row = k_subset.iloc[k_idx] | |||
| matchless.append(k_row) | |||
There was a problem hiding this comment.
...and add k_not_matchable to matchless here?
678042c to
a514429
Compare
a514429 to
877e347
Compare
|
Folded most of the comments into #78 closing in favour of that PR, thanks @robinmessage ! |
2 participants
This code fixes the distributions for the pair matchings. There might be a faster way of doing this but I feel it does the "right" thing. @robinmessage perhaps you could take a look at this quickly with your eye for detail and optimisation :)) ? It also has the benefit that should we want to we can dump the exact potentials that match and don't match per K which was useful during all the debugging.