make equation behind torch.isclose element-wise #138459
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138459
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit 9762576 with merge base 14fc6b7 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
the
release notes: python_frontend
soulitzer
left a comment
soulitzer
left a comment
There was a problem hiding this comment.
I'm not sure this is clearer. Adding the "forall" here feels like could be confused with the behavior of allclose.
soulitzer
added
the
triaged
|
Hi @soulitzer, thanks for stepping in! In case you refer to the equation that is in the docs for |
torch/_torch_docs.py
Outdated
| .. math:: | ||
| \lvert \text{input} - \text{other} \rvert \leq \texttt{atol} + \texttt{rtol} \times \lvert \text{other} \rvert | ||
| \lvert \text{input}_i - \text{other}_i \rvert \leq \texttt{atol} + \texttt{rtol} \times \lvert \text{other}_i \rvert \quad \forall i |
There was a problem hiding this comment.
Can we remove \forall ?
I do agree that it makes sense to add the subscript, e.g. to match https://pytorch.org/docs/stable/generated/torch.add.html#torch.add and other element-wise type operations.
But for me that forall seems to indicate that the function will return True/False based on whether EVERY element-wise comparison returned true.
| \lvert \text{input}_i - \text{other}_i \rvert \leq \texttt{atol} + \texttt{rtol} \times \lvert \text{other}_i \rvert \quad \forall i | ||
| """ | ||
| + r""" | ||
| elementwise, for all elements of :attr:`input` and :attr:`other`. The behaviour of this function is analogous to |
There was a problem hiding this comment.
IMO \forall does make sense for allclose, but it is already mentioned here in text. We should keep one or the other.
|
@soulitzer, do you have any other suggestions? |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
|
Hi @soulitzer, I cannot explain to myself why these 2 checks failed, given that we modified just the docstrings. Could you please indicate to me what would be the next step? |
|
@vladimirrotariu those tests should be unrelated :) I will retry the land |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 2 checks: pull / linux-jammy-py3-clang12-executorch / test (executorch, 1, 1, linux.2xlarge), trunk / linux-focal-cuda12.4-py3.10-gcc9-sm86 / test (default, 3, 5, linux.g5.4xlarge.nvidia.gpu) Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
The current formula behind torch.isclose, according to the docs, is  However, torch.isclose acts element-wise, so this formula may be misleading at first, given that the docs said that `input` and `other` are the first, respectively second tensor to compare. I propose the following change, to stress the element-wise nature of the norms in the equation:  Pull Request resolved: pytorch#138459 Approved by: https://github.com/soulitzer
4 participants
The current formula behind torch.isclose, according to the docs, is
However, torch.isclose acts element-wise, so this formula may be misleading at first, given that the docs said that
inputandotherare the first, respectively second tensor to compare. I propose the following change, to stress the element-wise nature of the norms in the equation: