[primTorch] Minor improvements to doc and impl of `gaussian_nll_loss` #85612

nkaretnikov · 2022-09-25T18:03:13Z

Stack from ghstack:

-> [primTorch] Minor improvements to doc and impl of gaussian_nll_loss #85612

Fixes #53392.

cc @ezyang @mruberry @ngimel @lezcano @peterbell10

[ghstack-poisoned]

pytorch-bot · 2022-09-25T18:03:15Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85612

📄 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

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit bd18a12:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

ghstack-source-id: e6ae96ccb06d9f070d87c55daf3a9b7b5bf3e140 Pull Request resolved: #85612

…uts" [ghstack-poisoned]

ghstack-source-id: a3dad8e4d7bd1d8e86a2dff56bd60468eb507e65 Pull Request resolved: #85612

nkaretnikov · 2022-09-25T18:17:30Z

Note: see #53964 and #56469 for context on the size checks and clamping before the loss calculation. The ref code is almost a 1-to-1 copy of the Python implementation.

…uts" [ghstack-poisoned]

ghstack-source-id: 149d24524a2f38c2195efaeb6bbf6e74fff729b1 Pull Request resolved: #85612

…uts" [ghstack-poisoned]

ghstack-source-id: bb6522bc64188cc25ec32173ee8a670d2d193218 Pull Request resolved: #85612

lezcano

This is an interesting one. This function is already implemented in Python in core, so it's not clear to me whether we want to re-implement it in PrimTorch. The only benefit I see in doing this is that we can implement some promotion rules for it, and the fact that this way we would have all our implementations in the same place... WDYT @mruberry? Do

torch/_refs/nn/functional/__init__.py

torch/testing/_internal/common_methods_invocations.py

torch/_refs/nn/functional/__init__.py

mruberry · 2022-09-27T15:21:17Z

torch/nn/functional.py

@@ -2777,8 +2777,10 @@ def gaussian_nll_loss(
    Args:
        input: expectation of the Gaussian distribution.
        target: sample from the Gaussian distribution.
-        var: tensor of positive variance(s), one for each of the expectations
-            in the input (heteroscedastic), or a single one (homoscedastic).
+        var: same shape as the input, or same shape as the input but with the


It's cool this PR is updating the documentation for the function.

When describing the parameters it's important to start with what's most important about them. In this case, I don't think it's the shape of var that's most important, but that var is a tensor describing the variances of either a multivariate normal distribution or multiple independent distributions (see question above).

This documentation also seems a little odd to me because input and target refer to "the Gaussian distribution", but this seems wrong because

the loss can be used for multiple Gaussian distributions simultaneously (because it supports batches)

I believe the correct semantic interpretation for this loss is that it works on multivariate normal distributions OR multiple normal distributions, and not just one?

So there may be more we can do here

I just removed the doc from the functional part. See the above comment WRT the Gaussian bit.

nkaretnikov · 2022-09-27T22:07:01Z

reminder to close this one once we fix the docs: #53392

…n_nll_loss`" [ghstack-poisoned]

ghstack-source-id: 86473881086564d9b4f502b9325dadac7bc83643 Pull Request resolved: #85612

…n_nll_loss`" Fixes #53392. [ghstack-poisoned]

Fixes #53392. ghstack-source-id: 351e85882f2035d50cdf09572d0374a5dcd7f39a Pull Request resolved: #85612

nkaretnikov · 2022-09-29T21:31:58Z

torch/nn/functional.py

@@ -2817,14 +2817,15 @@ def gaussian_nll_loss(
        raise ValueError(reduction + " is not a valid value for reduction")

    # Clamp for stability
-    var = var.clone()


not sure what was the purpose of cloning here since the same variable is used later anyway

clone + in-place was a worse version of doing clamp out of place I think.

Now I know why. It's either the original behavior (with no_grad and inplace clamp) or just this (without any context): var = var.clamp(min=eps). I couldn't find any other code that wouldn't break the following tests:

python -m pytest test/test_modules.py -k GaussianNLLLoss -vvv python -m pytest test/test_ops_gradients.py -k gaussian_nll_loss -vvv

This claims that doing it without no_grad will "cause divergence," but the tests pass locally.

any thoughts @albanD ? Clamping without no_grad LGTM, but I know we've historically done it with the no_grad...

clamping without no_grad will zero out a bunch of gradients. We definitely don't want that!

What about figuring out what's the value we want this function to take at var = 0 and return that? Although we would need to somehow deal with the gradients as well...

Note: @lezcano told me offline there are plans (IIUC) to have a "framework" that would help with numerical issues like this one, so postponing for now.

my point is that, at the moment, we don't care about gradients on PrimTorch (just yet). But we do care about gradients in PyTorch. As such, given that this is a function exposed in PyTorch, it should be correct. In particular, this change makes the gradients of this function to be incorrect and should be reverted.

Then, we should revisit at some point how to approach the whole point of the gradients in PrimTorch.

functorch/test/test_ops.py

lezcano · 2022-09-30T07:59:35Z

torch/nn/functional.py

@@ -2817,14 +2817,15 @@ def gaussian_nll_loss(
        raise ValueError(reduction + " is not a valid value for reduction")

    # Clamp for stability
-    var = var.clone()


clone + in-place was a worse version of doing clamp out of place I think.

…n_nll_loss`" Fixes #53392. [ghstack-poisoned]

Fixes #53392. ghstack-source-id: b624b8533e25af07e503849acae0c5c7d9865e1f Pull Request resolved: #85612

…n_nll_loss`" Fixes #53392. [ghstack-poisoned]

Fixes #53392. ghstack-source-id: c0f85f80b031f5c287440c98748ad82c7768851e Pull Request resolved: #85612

nkaretnikov · 2022-10-01T08:17:17Z

@lezcano @mruberry PTAL.

Summary:

removed no_grad from clamp and changed to functional clamp
removed a call to .item, which resulted in better test coverage
improved the docs to document allowed var inputs
this doesn't add a ref (based on our discussion in another PR) because it's a pure Python impl
added error inputs.

facebook-github-bot · 2022-10-04T00:13:14Z

/easycla

As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details.

This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign.

linux-foundation-easycla · 2022-10-04T00:13:31Z

❌ - login: @nkaretnikov / name: Nikita Karetnikov . The commit (295c92d, 3b0933f, f621835, c48f39a, 883d605, 686d75a, ae50916, 5273357, bd18a12) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

mruberry · 2022-10-05T02:27:05Z

torch/nn/functional.py

@@ -2762,6 +2762,7 @@ def poisson_nll_loss(
    return ret


+# TODO: Pure Python impl - don't add a primTorch ref


I would remove this comment -- it's not really a TODO since there's nothing to do

mruberry · 2022-10-05T02:27:56Z

torch/nn/functional.py

+    # If var is the same shape as input, it's the heteroscedastic case.
+    # If var is *not* the same shape as input, it's the homoscedastic case.
+    #
+    # To support broadcasting, the following sub-cases are allowed in the


It's not really broadcasting, though. We should be clear about what, exactly, it does, and not use a similar concept, which could confuse the reader

mruberry · 2022-10-05T02:29:23Z

torch/nn/modules/loss.py

@@ -314,13 +314,13 @@ class GaussianNLLLoss(_Loss):
    where :attr:`eps` is used for stability. By default, the constant term of
    the loss function is omitted unless :attr:`full` is ``True``. If ``var`` is not the same
    size as ``input`` (due to a homoscedastic assumption), it must either have a final dimension
-    of 1 or have one fewer dimension (with all other sizes being the same) for correct broadcasting.
+    of 1 or have one fewer dimension (when comparing from the outermost dimension, with all other
+    sizes being the same) for correct later broadcasting.


Let's not use the term "broadcasting" here because this is not the same thing

mruberry · 2022-10-05T02:29:39Z

torch/nn/modules/loss.py

@@ -314,13 +314,13 @@ class GaussianNLLLoss(_Loss):
    where :attr:`eps` is used for stability. By default, the constant term of
    the loss function is omitted unless :attr:`full` is ``True``. If ``var`` is not the same
    size as ``input`` (due to a homoscedastic assumption), it must either have a final dimension


I don't think the phrase "final dimension" is commonly understood

mruberry · 2022-10-05T02:32:10Z

torch/nn/modules/loss.py

@@ -314,13 +314,13 @@ class GaussianNLLLoss(_Loss):
    where :attr:`eps` is used for stability. By default, the constant term of
    the loss function is omitted unless :attr:`full` is ``True``. If ``var`` is not the same
    size as ``input`` (due to a homoscedastic assumption), it must either have a final dimension
-    of 1 or have one fewer dimension (with all other sizes being the same) for correct broadcasting.
+    of 1 or have one fewer dimension (when comparing from the outermost dimension, with all other


I would just break this into cases. I'm not sure the order of comparison makes it that clear. Either var has the same shape, has the same shape except its innermost dimension is 1, or has the same shape except it's "missing" the innermost dimension. In the last two cases the variance is assumed to be the same for each distribution (the distributions are assumed to be homoscedastic)

mruberry

The functional and test changes look good, but I think we should take the time to be more diligent with the docs. It could save us a lot of headaches in the future.

One option would be to separate the doc changes into another PR.

mruberry · 2022-11-07T16:05:00Z

torch/nn/functional.py

@@ -2773,19 +2774,6 @@ def gaussian_nll_loss(
    r"""Gaussian negative log likelihood loss.

    See :class:`~torch.nn.GaussianNLLLoss` for details.
-


I would keep this.

While we do like to reduce document redundancy, in a more perfect world modules would just wrap functions, and the functions would be documented.

github-actions · 2023-01-10T19:33:40Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

[primTorch] Add ref for gaussian_nll_loss, add error inputs

295c92d

[ghstack-poisoned]

facebook-github-bot added the cla signed label Sep 25, 2022

nkaretnikov added a commit that referenced this pull request Sep 25, 2022

[primTorch] Add ref for gaussian_nll_loss, add error inputs

3f81987

ghstack-source-id: e6ae96ccb06d9f070d87c55daf3a9b7b5bf3e140 Pull Request resolved: #85612

Update on "[primTorch] Add ref for gaussian_nll_loss, add error inp…

3b0933f

…uts" [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 25, 2022

[primTorch] Add ref for gaussian_nll_loss, add error inputs

ae5fcce

ghstack-source-id: a3dad8e4d7bd1d8e86a2dff56bd60468eb507e65 Pull Request resolved: #85612

pytorchbot added the open source label Sep 25, 2022

Update on "[primTorch] Add ref for gaussian_nll_loss, add error inp…

f621835

…uts" [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 25, 2022

[primTorch] Add ref for gaussian_nll_loss, add error inputs

9d7d690

ghstack-source-id: 149d24524a2f38c2195efaeb6bbf6e74fff729b1 Pull Request resolved: #85612

Update on "[primTorch] Add ref for gaussian_nll_loss, add error inp…

c48f39a

…uts" [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 25, 2022

[primTorch] Add ref for gaussian_nll_loss, add error inputs

b00aaa1

ghstack-source-id: bb6522bc64188cc25ec32173ee8a670d2d193218 Pull Request resolved: #85612

nkaretnikov marked this pull request as ready for review September 26, 2022 08:49

nkaretnikov requested review from mruberry, ngimel, albanD and jbschlosser as code owners September 26, 2022 08:49

nkaretnikov requested review from lezcano and removed request for albanD, ngimel, mruberry and jbschlosser September 26, 2022 08:49

nkaretnikov added topic: not user facing topic category module: primTorch labels Sep 26, 2022

lezcano reviewed Sep 26, 2022

View reviewed changes

mruberry self-requested a review September 26, 2022 13:22

mruberry reviewed Sep 26, 2022

View reviewed changes

torch/testing/_internal/common_methods_invocations.py Show resolved Hide resolved

mruberry reviewed Sep 26, 2022

View reviewed changes

torch/testing/_internal/common_methods_invocations.py Show resolved Hide resolved

mruberry reviewed Sep 26, 2022

View reviewed changes

torch/_refs/nn/functional/__init__.py Outdated Show resolved Hide resolved

mruberry reviewed Sep 26, 2022

View reviewed changes

torch/_refs/nn/functional/__init__.py Outdated Show resolved Hide resolved

mruberry reviewed Sep 27, 2022

View reviewed changes

torch/_refs/nn/functional/__init__.py Outdated Show resolved Hide resolved

mruberry reviewed Sep 27, 2022

View reviewed changes

nkaretnikov changed the title ~~[primTorch] Add ref for gaussian_nll_loss, add error inputs~~ [primTorch] Minor improvements to doc and impl of gaussian_nll_loss Sep 28, 2022

Update on "[primTorch] Minor improvements to doc and impl of `gaussia…

686d75a

…n_nll_loss`" [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 28, 2022

[primTorch] Minor improvements to doc and impl of gaussian_nll_loss

ee75803

ghstack-source-id: 86473881086564d9b4f502b9325dadac7bc83643 Pull Request resolved: #85612

Update on "[primTorch] Minor improvements to doc and impl of `gaussia…

ae50916

…n_nll_loss`" Fixes #53392. [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 29, 2022

[primTorch] Minor improvements to doc and impl of gaussian_nll_loss

d0ccfb4

Fixes #53392. ghstack-source-id: 351e85882f2035d50cdf09572d0374a5dcd7f39a Pull Request resolved: #85612

nkaretnikov commented Sep 29, 2022

View reviewed changes

lezcano reviewed Sep 30, 2022

View reviewed changes

Update on "[primTorch] Minor improvements to doc and impl of `gaussia…

5273357

…n_nll_loss`" Fixes #53392. [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 30, 2022

[primTorch] Minor improvements to doc and impl of gaussian_nll_loss

cf44de1

Fixes #53392. ghstack-source-id: b624b8533e25af07e503849acae0c5c7d9865e1f Pull Request resolved: #85612

Update on "[primTorch] Minor improvements to doc and impl of `gaussia…

bd18a12

…n_nll_loss`" Fixes #53392. [ghstack-poisoned]

nkaretnikov added a commit that referenced this pull request Sep 30, 2022

[primTorch] Minor improvements to doc and impl of gaussian_nll_loss

314e263

Fixes #53392. ghstack-source-id: c0f85f80b031f5c287440c98748ad82c7768851e Pull Request resolved: #85612

mruberry reviewed Oct 5, 2022

View reviewed changes

nkaretnikov mentioned this pull request Nov 4, 2022

Add error inputs to gaussian_nll_loss OpInfo #88486

Closed

mruberry reviewed Nov 7, 2022

View reviewed changes

nkaretnikov marked this pull request as draft November 11, 2022 19:30

github-actions bot added the Stale label Jan 10, 2023

github-actions bot closed this Feb 9, 2023

facebook-github-bot deleted the gh/nkaretnikov/6/head branch June 8, 2023 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[primTorch] Minor improvements to doc and impl of `gaussian_nll_loss` #85612

[primTorch] Minor improvements to doc and impl of `gaussian_nll_loss` #85612

nkaretnikov commented Sep 25, 2022 •

edited by pytorch-bot bot

pytorch-bot bot commented Sep 25, 2022 •

edited

nkaretnikov commented Sep 25, 2022 •

edited

lezcano left a comment

mruberry Sep 27, 2022 •

edited

nkaretnikov Sep 28, 2022

nkaretnikov commented Sep 27, 2022

nkaretnikov Sep 29, 2022 •

edited

lezcano Sep 30, 2022

nkaretnikov Sep 30, 2022

lezcano Oct 1, 2022

albanD Oct 3, 2022 •

edited

lezcano Oct 3, 2022

nkaretnikov Nov 4, 2022

lezcano Nov 4, 2022

lezcano Sep 30, 2022

nkaretnikov commented Oct 1, 2022 •

edited

facebook-github-bot commented Oct 4, 2022

linux-foundation-easycla bot commented Oct 4, 2022

mruberry Oct 5, 2022

mruberry Oct 5, 2022

mruberry Oct 5, 2022

mruberry Oct 5, 2022

mruberry Oct 5, 2022

mruberry left a comment

mruberry Nov 7, 2022

github-actions bot commented Jan 10, 2023

		@@ -2762,6 +2762,7 @@ def poisson_nll_loss(
		return ret


		# TODO: Pure Python impl - don't add a primTorch ref

		@@ -2773,19 +2774,6 @@ def gaussian_nll_loss(
		r"""Gaussian negative log likelihood loss.

		See :class:`~torch.nn.GaussianNLLLoss` for details.

[primTorch] Minor improvements to doc and impl of gaussian_nll_loss #85612

[primTorch] Minor improvements to doc and impl of gaussian_nll_loss #85612

Conversation

nkaretnikov commented Sep 25, 2022 • edited by pytorch-bot bot

pytorch-bot bot commented Sep 25, 2022 • edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85612

✅ No Failures

nkaretnikov commented Sep 25, 2022 • edited

lezcano left a comment

Choose a reason for hiding this comment

mruberry Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkaretnikov commented Sep 27, 2022

nkaretnikov Sep 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albanD Oct 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkaretnikov commented Oct 1, 2022 • edited

facebook-github-bot commented Oct 4, 2022

linux-foundation-easycla bot commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2023

[primTorch] Minor improvements to doc and impl of `gaussian_nll_loss` #85612

[primTorch] Minor improvements to doc and impl of `gaussian_nll_loss` #85612

nkaretnikov commented Sep 25, 2022 •

edited by pytorch-bot bot

pytorch-bot bot commented Sep 25, 2022 •

edited

nkaretnikov commented Sep 25, 2022 •

edited

mruberry Sep 27, 2022 •

edited

nkaretnikov Sep 29, 2022 •

edited

albanD Oct 3, 2022 •

edited

nkaretnikov commented Oct 1, 2022 •

edited