| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 24.7k
make equation behind torch.isclose element-wise #138459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| @@ -5329,7 +5329,7 @@ def merge_dicts(*dicts): | |||
| Closeness is defined as: | |||
|
|
|||
| .. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -740,7 +740,7 @@ def merge_dicts(*dicts): | |||
| This function checks if :attr:`input` and :attr:`other` satisfy the condition: | |||
|
|
|||
| .. 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 | |||
| """ | |||
| + 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO \forall does make sense for allclose, but it is already mentioned here in text. We should keep one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soulitzer now I see what you mean :)
|
@soulitzer, do you have any other suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
|
@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
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: