Make MaxAndArgmax use internally signed axis numbers.#279
Make MaxAndArgmax use internally signed axis numbers.#279abergeron merged 2 commits intoTheano:masterfrom
Conversation
This avoids an infinite loop when counting ndX downto 0 with unsigned integers, as the condition `unsigned >= 0` always holds.
|
jenkins test this |
|
if you can add a test that was failing and now pass, it would be great. I let @abergeron check in more detail the code. |
|
jenkins test this |
|
Nothing was failling before since there was a check on the arguments to prevent it. However it made the reduce-all-axes case return an error. |
| case 0: | ||
| ctx->err = cuLaunchKernel(k->k, 1, 1, 1, 1, 1, 1, shared, | ||
| ctx->s, args, NULL); | ||
| break; |
There was a problem hiding this comment.
I'm not sure we want to support 0-dim at this level. This is basically useless.
If you really need it special case it where you do the kernel call.
There was a problem hiding this comment.
@abergeron I amended the commit, that file is no longer touched. However, I do think it made sense. The pattern is pretty clear:
- 3-dims is a volume XYZ
- 2-dims is a matrix XY with Z=1
- 1-dims is a vector X with Y=Z=1
- 0-dims is a scalar with X=Y=Z=1
There was a problem hiding this comment.
The pattern is clear but running a 1, 1, 1 kernel is almost always a mistake and I don't want to encourage it.
All-dims-reduced will be slow but does work now without errors. Added testcase to ensure this remains the case.
3 participants
This avoids an infinite loop when counting ndX downto 0 with unsigned
integers, as the condition
unsigned >= 0always holds.