🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/78515
• 📄  Preview Python docs built from this PR
• 📄  Preview C++ docs built from this PR
• ❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit 04b1541 (more details on the Dr. CI page):

This is not about the cast but creating an empty({0}) and resizing it down the line.

Previously, out variant used to perform the computation frobenius_norm_impl which returns a new Tensor and copy it into resized out (Resizing a CCT is not valid).

frobenius_norm_impl pulls the common logic of actually computing the norm and returns a Tensor which

• functional variant: returns after casting
• out variant: copies into out (after resizing).

Feels like this PR needs more comments 😅

I think the comment above still applies.

1. I think that all the logic should just be in frobenius, and then frobenius_out should call into it. Otherwise you may have a divergence between the out-of-place and the out function, as we do now.
2. I do not see why the cast to the real numbers or the point is necessary as the tensor by that point is already real, but then again, no need to fix this now.

@lezcano, what is your proposal here? Is it to rewrite the code like the following?

Tensor frobenius_norm(const Tensor& self, IntArrayRef dim, bool keepdim) {
 ...
}

Tensor &frobenius_norm_out(const Tensor& self,
    IntArrayRef dim,
    bool keepdim,
    Tensor& result) {
  auto output = frobenius(self, dim, keepdim);
  result.resize_(output.sizes());
  return result.copy_(output);
}

Yup! This is the pattern we use in many other composite operations.

@zou3519 @lezcano PTAL :)

Requiring the cast was by-product of having implemented frobenius_norm_impl. With current approach that is not necessary. I don't think we need to file an issue for the same with the approach as suggested by @lezcano. We also have asserts to verify that we don't change the expected layout and output dtype.

@pytorchbot merge

@pytorchbot successfully started a merge job. Check the current status here

@kshitij12345 your PR has been successfully merged.

Hey @kshitij12345.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.