How do I change this so that gcd gets invoked only with integral arguments?

@mruberry

You don't need to add gcd or lcm to common methods invocations, since it's mostly intended for testing gradients, which you're not dealing with.

Okay. In that case, I'm ready for a review.

Newline

What does this return if a is zero? If b is zero? If both are zero?

(We should just add a comment explaining the behavior, since we can't return NaN for gcd(0, 0) here)

I suppose we could return an optional, but that seems like a lot.

Also you can use the same type traits trick as calc_erfinv (with is_integral instead of is_floating_point):

https://github.com/pytorch/pytorch/blob/40228ef2e59dced45b1b39819e57ea150e14b4ce/aten/src/ATen/native/Math.h#L66

If a is 0, we return b (this is the case even when b is also 0). If b is 0 and a is nonzero then the while loop is executed once, b takes the value of a and is returned. In short, this will never return NaN, with gcd(a, 0) = a; gcd(0, b) = b; gcd(0, 0) = 0.

Good. I just want to be clear about the (0, 0) behavior.

You can use:

All other functions in that header (Math.cuh) file also don't use that macro. Should I change them too?

No. Old code that's working is best left undisturbed. Thank you for asking, though.

Is the accumulate type ever different for the integer types?

I thought accscalar_t stood for "accelerated scalar type" (as in accelerated with CUDA), my bad. I'll change that.

Let's remove this for now. Named tensor support is on the back burner and these functions don't directly support named tensors since they're implementing a new kernel.

For the tolerances here, can they not all be 0?

Yes, you're right.

"value"->"other" to be consistent with the documentation below?

"-> Tensor", not "LongTensor"

"LongTensor" -> "Tensor"

"... of input and other."

Let's be pedantic and be clear the function defines gcd(0, 0)=0.

"Both input and other must have integer dtypes."

See above notes.

In particular, what is lcm(a, 0)?

Should there be a test in test_torch.py explicitly for the gcd and lcm edge cases where one of the two inputs is zero and when both of the inputs are zero?

Yeah, I think having explicit tests is a good idea (I tested these edge cases against the numpy implementation manually). And, lcm(a, 0) = 0 (which is also the case in the numpy implementation).

On second thought, doesn't this line already account for this?

You would think so because our test suite is so confusing, but I think that function is for the Math tests and your tests are in the method test section. I'm actually about to overhaul all of this so it's not super important where you test the extremal values. You can probably just add lcm to the math test portion, but some people have struggled to fit their functions in there. You would do that by adding to torch_op_tests on line 19546.