I thought it had to be CompositeImplicitAutograd for automatic backward?

For some reason, although we have an explicit addr derivative formula XLA didn't want to use it. I'm forcing XLA to use our old formula now and I want to see what happens

XLA tests passed so looks like we're good to go here

Wouldn't expand fail if self.ndim wasn't 2?

I guess we should update the original code lol

same here, isn't it guaranteed by expand?

should you also check alpha dtype (it should be of the same type kind as other inputs)?
@mruberry do you want to make a case again for alpha checking being done in the wrapper?

Integers are OK though

>>> torch.addr(torch.tensor([[False]]), torch.tensor([False]), torch.tensor([True]), beta=1, alpha=2)
tensor([[False]])

Extending the type promotion wrapper would probably make life easier

I think we should say that integer numbers can interact with bools and it's fine, however

I tried the obvious thing which is

@elementwise_type_promotion_wrapper(
    type_promoting_args=("self", "vec1", "vec2", "beta", "alpha"),
    type_promotion_kind=ELEMENTWISE_TYPE_PROMOTION_KIND.DEFAULT,
)       

but this doesn't work as an integer alpha causes the entire tensor to promote to int64. I'm not sure what the recommend spelling of this change would be, it seems to have far reaching effects.

This unconditional expand seems odd -- maybe we can use a construct like _maybe_expand_to?

Here is the original code:

  check_1d(vec1, "vec1", "addr");
  check_1d(vec2, "vec2", "addr");
    
  const auto vec1_size0 = vec1.sizes()[0];
  const auto vec2_size0 = vec2.sizes()[0];
  auto self_ = &result == &self
    ? c10::MaybeOwned<Tensor>::borrowed(self)
    : expand_size(self, {vec1_size0, vec2_size0}, "addr");
  TORCH_CHECK(
    self_->dim() == 2,
    "2D tensor expected, got ", self_->dim(), "D tensor for input"
  );    
  TORCH_CHECK(
    self_->sizes()[0] == vec1_size0 && self_->sizes()[1] == vec2_size0,
    "size mismatch, input: ", self_->sizes(),
    ", v1: ", vec1.sizes(),
    ", v2: ", vec2.sizes()
  );

It looks like the expand is done unconditionally, so I faithfully represented this. Are we expected to simplify the logic when writing refs?

It'd be nice if we improve it, sure -- if we don't do it now it's just more time and review later

_maybe_expand_to is... not a thing?

How do you mean?

Won't the expand have already asserted this?

Once again, faithful representation of the original code :) My rule for porting: first do a slavish port, and then once that passes all tests, then refactor.

haha, OK I guess -- but some cleanups seem easy to snipe

I don't think these decompositions for addr are correct. When beta==0, for example, NaNs in self should not propagate

(this will require adding reference inputs for addr that test the NaN propagation, too!)

btw, in the original code, even if alpha==0 the NaNs still propagate

Eeeks! We should probably get that mode implemented before we start writing refs with torch functions if they have to skip the reference consistency test

Well, this is what you asked for, but you are welcome to change your mind ;)

We will have a context manager that will reroute torch.* API calls to torch._ref.* (failing if the ref doesn't exist),

and

do you have a preference for two separate tests (but then I need to dupe all the xfails for the new test name I added) or one test (but then an xfail will prevent us checking that the torch.* version works)
one test if it’s easier

My preferred resolution is to maintain only one test which does NOT test with the mode (so torch goes to torch, refs go to refs), and not test anything else. If we insist on testing the strict mode, we will need to add an extra set of tests, because any ref that calls into a torch function that doesn't have a ref will fail in strict mode. We can also test in non-strict mode, in which case you don't get any signal if all torch functions being called have refs or not.

Yes I have a PR that will be available soon that separates the tests, though, so we can still test for consistency even without refs

FYI #78026 has landed updating and separating the tests (may require a rebase)