-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Remove .data from benchmarks and tensorboard
#65389
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
Remove .data from benchmarks and tensorboard
#65389
Conversation
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 87ea5d3 (more details on the Dr. CI page):
🕵️ 2 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
| Job | Step | Action |
|---|---|---|
| Unknown | 🔁 rerun |
This comment was automatically generated by Dr. CI (expand for details).
Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group.
albanD
left a comment
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.
Thanks for the cleanup!
torch/utils/tensorboard/summary.py
Outdated
| wave_write.setsampwidth(2) | ||
| wave_write.setframerate(sample_rate) | ||
| wave_write.writeframes(tensor.data) | ||
| wave_write.writeframes(tensor.detach()) |
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.
This sometimes get a numpy array actually. So we should not unconditionally call this.
Not sure what '.data' does on numpy arrays though...
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.
Indeed. I think we can just remove .detach(), because there is a call to make_np(tensor) already. If that's ok, I can update. BTW, should we also update the docstring (of both audio and video) to something like this?
pytorch/torch/utils/tensorboard/writer.py
Line 566 in 9324181
| img_tensor (torch.Tensor, numpy.array, or string/blobname): Image data |
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.
Actually .data is a valid thing on numpy array: https://numpy.org/doc/stable/reference/generated/numpy.ndarray.data.html
So this one I think we just want to rename tensor to array and keep the .data!
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.
Although .data is valid, it is not required in this case. It returns a memoryview, and id(memoryview(x)) == id(x.data). As wave gets a memoryview in both cases (see here), and the test without .data passes, I guess we could simply remove it. Anyway, I think we can proceed as you're suggesting, because at this point, given make_np call at the beginning, it'll be a numpy array. I submitted the change.
albanD
left a comment
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.
Thanks for the update. Looks good now.
|
@albanD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Related to #30987 and #33628. Fix the following tasks:
.datain all our internal code:benchmarks/torch/utils/tensorboard/cc @pietern @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @SciPioneer @H-Huang @gcramer23 @albanD @gchanan