[New Operator] Implement Mod operator#2345
Conversation
jackm321
force-pushed
the
implement_mod
branch
2 times, most recently
from
5b1912d to
8eb384f
Compare
There was a problem hiding this comment.
Let's simulate Caffe2 behavior. This implementation diverges at least when res == 0.
https://github.com/pytorch/pytorch/blob/1409a2afc8d81d911dbcfc2d0b210cf62658237b/caffe2/operators/mod_op.cc#L20
There was a problem hiding this comment.
@artemrakhov this implementation is equivalent to the code you linked and skips the unnecessary divisor > 0 check.
There was a problem hiding this comment.
I don't agree that it is equivalent. They never touch 0. But this code adds divisor to 0, making result of 0 % 3 to be 3. And the unit test #2 that you've added confirms this.
There was a problem hiding this comment.
Yeah you're right about the output_ptr[i] check, I'll add that to this too
jackm321
force-pushed
the
implement_mod
branch
from
8eb384f to
c07b215
Compare
There was a problem hiding this comment.
from the doc: Input tensor with int32 or int64 data.
This will fail in debug mode if tensor of int32 data.
There was a problem hiding this comment.
Good point. Do we have an existing solution for this? Looks like dispatchFloatingPointImpl, dispatchQuantizedImpl, and dispatchQuantizedWithAccumulationImpl don't quite fit this.
There was a problem hiding this comment.
Yeah, Int32ITy and Int64ITy are indexed types, not quantized types. So, you'd need to add something similar.
There was a problem hiding this comment.
I don't agree that it is equivalent. They never touch 0. But this code adds divisor to 0, making result of 0 % 3 to be 3. And the unit test #2 that you've added confirms this.
|
@jackm321 @artemrakhov @artemrakhov Is there a reason why we are not templating all of the binary operators? Is there a difference between Mul, Sub, Add except to the one character that specifies the op? |
|
@nadavrot: This op is different, because of We do have a lot of templates around Add, Mul, Sub, Div. Quantization formulas are different, that's why it's not one character difference. I admit we could use more templates for these. |
jackm321
force-pushed
the
implement_mod
branch
from
1570379 to
84dbf25
Compare
|
@rdzhabarov is there any reason not to merge |
For many cases (ops) there are no quantized impl for a certain op. I think it makes sense to keep them separately. |
rdzhabarov
left a comment
rdzhabarov
left a comment
There was a problem hiding this comment.
Looks good from my side.
lib/Importer/Caffe2ModelLoader.cpp
Outdated
| int64_t divisor; | ||
| ASSIGN_VALUE_OR_RETURN_ERR(divisor, loadInt(dict["divisor"])); | ||
|
|
||
| RETURN_ERR_IF_NOT(divisor >= 1, "Divisor must be positive."); |
|
@artemrakhov good to go? |
| loadInt(dict["sign_follow_divisor"])); | ||
| } | ||
|
|
||
| auto *node = G_.createModulo(opName, in, divisor, signFollowDivisor); |
There was a problem hiding this comment.
At this point divisor is silently converted from 64-bit to 32-bit. Is it intentional? Caffe2 uses int64_t
jackm321
force-pushed
the
implement_mod
branch
from
c3a18f3 to
5bf8521
Compare
|
Thanks @rdzhabarov and @artemrakhov for reviewing! |
5 participants
Description:
Implements caffe2 Mod operator.
I chose to call the IR node/instruction Modulo instead of Mod to avoid confusion with Modules which are referred to throughout the codebase as mod.
Testing:
ninja testDocumentation:
doxygen
Fixes #2330