fix upper-bound for local-region in lrn test case #1095
Conversation
| for n, c, h, w in np.ndindex(x.shape): | ||
| square_sum[n, c, h, w] = sum(x[n, | ||
| max(0, c - int(math.floor((nsize - 1) / 2))):min(4, c + int(math.ceil((nsize - 1) / 2)) + 1), | ||
| max(0, c - int(math.floor((nsize - 1) / 2))):min(5, c + int(math.ceil((nsize - 1) / 2)) + 1), |
There was a problem hiding this comment.
no I tested this on both numpy and with mxnet as backend. here 5 is the size of the array and 4 is the index. infact having 5 here will give ArrayIndexOutOfBoundsException.
There was a problem hiding this comment.
As I understand, numpy indexing is upper-bound exclusive. You mentioned previously that the "test case has a +1 because numpy indexing is upper-bound exclusive." The +1 appears only in the second argument of "min". Shouldn't you either move the +1 outside the min or, equivalently, have a +1 for both arguments of "min"?
There was a problem hiding this comment.
When I run this modified version, it works fine for me. If I use the original value of 4, I get incorrect results. Is it possible that you have different parenthesization in your mxnet version (with +1 outside min) than in the above file?
There was a problem hiding this comment.
I agree with @gramalingam, since the upper bound is exclusive, we should use C (i.e., 5) instead of C-1 in this case. I locally tested the script, it worked.
There was a problem hiding this comment.
@gramalingam yes in my test example, I had the +1 outside the parentheses.
houseroad
left a comment
houseroad
left a comment
There was a problem hiding this comment.
@gramalingam also update the generated input and output data (.pb files) please.
| for n, c, h, w in np.ndindex(x.shape): | ||
| square_sum[n, c, h, w] = sum(x[n, | ||
| max(0, c - int(math.floor((nsize - 1) / 2))):min(4, c + int(math.ceil((nsize - 1) / 2)) + 1), | ||
| max(0, c - int(math.floor((nsize - 1) / 2))):min(5, c + int(math.ceil((nsize - 1) / 2)) + 1), |
There was a problem hiding this comment.
@gramalingam yes in my test example, I had the +1 outside the parentheses.
|
the window size parameter for LRN is supposed to take only odd values. Could you please update the spec? |
|
@anirudhacharya I think there is no need to forbid even values. |
* fix upper-bound for local-region in lrn test case * generate lrn test data output * Update the docs
* fix upper-bound for local-region in lrn test case * generate lrn test data output * Update the docs
4 participants
@anirudhacharya there seems to be an off-by-one error in the LRN test-case also (or parenthesization error).