DOC: Initialize numpy compatibility note #71688
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
|
|
Hi @HaoZeke! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
🔗 Helpful links
❌ 1 Flaky FailuresAs of commit 40af0c7 (more details on the Dr. CI page): Expand to see more✅ None of the CI failures appear to be your fault 💚
❄️ 1 failure tentatively classified as flakybut reruns have not yet been triggered to confirm:
|
6679cd3 to
935c282
Compare
There was a problem hiding this comment.
Thanks @HaoZeke, this is a good start.
A section "Using NumPy functions directly on PyTorch tensors" still seems to be missing. (concrete example: np.sin(t) works. let's explain why). The reductions special-case mentioned in gh-48628 is also missing.
Also note #46829 (comment). Here you use + as the operator, but it'd be good to copy that example and mention that numpy may behave unexpectedly for different operators.
e0ca917 to
218d0a1
Compare
8a45130 to
dd86ad9
Compare
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
|
||
| **TL;DR:** | ||
|
|
||
| - Be explicit about obtaining and using NumPy arrays |
There was a problem hiding this comment.
Having now read the document, I like being clear about converting between torch tensors and NumPy arrays. I think we should just tell people if they want to call NumPy operations on PyTorch tensors how they can do that (like creating a NumPy view of the data and running in a no grad context). We should let people know that PyTorch has a very "NumPy compatible" API and try to note the biggest divergences. We should talk about our Python Array API support (which is still in development) and let the user know if they can acquire the Python array API namespace from a torch tensor (not at this time).
48128bb to
7d765b0
Compare
|
@HaoZeke the warnings get turned into errors in CI. That's why there's no working link to the rendered docs here. Can you just put it in double backticks if it's not easy to figure out? The reference isn't essential I'd say. |
c78a030 to
6ba3626
Compare
**Reopened** to help with merge issues. See #59790 for full context. Fixes #20778. Helps #71688. Finalizes @martinPasen's force argument for `Tensor.numpy()`. It is set to False by default. If it's set to True then we: 1. detatch the Tensor, if requires_grad == True 2. move to cpu, if not on cpu already 3. Uses .resolve_conj() if .is_conj() == True 4. Uses .resolve_neg() if .is_neg() == True cc @albanD Pull Request resolved: #78564 Approved by: https://github.com/albanD
Summary: **Reopened** to help with merge issues. See #59790 for full context. Fixes #20778. Helps #71688. Finalizes martinPasen's force argument for `Tensor.numpy()`. It is set to False by default. If it's set to True then we: 1. detatch the Tensor, if requires_grad == True 2. move to cpu, if not on cpu already 3. Uses .resolve_conj() if .is_conj() == True 4. Uses .resolve_neg() if .is_neg() == True cc albanD Pull Request resolved: #78564 Approved by: https://github.com/albanD Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/3f58dd18dc6fc18ed82fb1632cea48373c0a7798 Reviewed By: seemethere Differential Revision: D36935606 Pulled By: seemethere fbshipit-source-id: dc2dd7f569feb8da29add55db3d1625241ff8d77
Also address most review comments, add doctests Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Co-authored-by: Mike Ruberry <mruberry@fb.com> Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Co-authored-by: Mike Ruberry <mruberry@fb.com> Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
There was a problem hiding this comment.
This now recommends in a couple of places to use .numpy(force=True), which is not a good idea. That feature is something that is meant for a use case like described in one of the issues: Napari wants to provide generic support for array/tensor libraries, and as a visualization library doesn't need to worry about performance as much or the implications of implicit device transfer or .detach(). For the average user on the other hand, we don't want to teach such bad habits and sweep all the details under the rug.
This still doesn't read well, it still feels like a long list of weird corner cases. Concrete example of that: there's a red warning box near the top with "Do not use id to check for shared memory", which just feels like a big distraction to the story (I have never seen any user do this in practice).
Most of the content is there, it just needs to be polished up with a focus on teaching, and hiding all the corner cases as notes in dropdown boxes instead of giving them center stage.
@HaoZeke I propose to put this on hold for now, for two reasons: (1) after multiple iterations it still feels off the mark, and (2) there are higher-prio things you have started on (primtorch). With a few hours to refactor this I think it's mergable, but that's probably something @mruberry or I should do.
@mruberry's main review comment from before was:
"Having now read the document, I like being clear about converting between torch tensors and NumPy arrays. I think we should just tell people if they want to call NumPy operations on PyTorch tensors how they can do that (like creating a NumPy view of the data and running in a no grad context). We should let people know that PyTorch has a very "NumPy compatible" API and try to note the biggest divergences. We should talk about our Python Array API support (which is still in development) and let the user know if they can acquire the Python array API namespace from a torch tensor (not at this time)."
It doesn't feel like that was addressed.
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
Fixes #48628, #46829.
@rgommers