-
Notifications
You must be signed in to change notification settings - Fork 432
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 *_devauto
functions
#1110
Conversation
@kpot Thanks a lot! I'll do a proper review in the coming days. As for the serialization system, I think the best way will be to introduce a device somewhere in the trait, but this can be done in a following PR as well. |
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 to me!
Just doing this refactor fixes some problems that we have, in mask generation, for instance, where the default device needed to be the same as the one passed as an argument. I'm pretty happy about the change, way harder to make a mistake and pretty clear where you need to pay attention to the device. Thanks a lot for this work.
We will wait for the CI to be fixed before merging, @syl20bnr.
@kpot CI is fixed in main, you can rebase this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1110 /- ##
==========================================
Coverage 85.52% 85.70% 0.18%
==========================================
Files 511 511
Lines 55079 55825 746
==========================================
Hits 47106 47845 739
- Misses 7973 7980 7 ☔ View full report in Codecov by Sentry. |
Done |
Checklist
run-checks all
script has been executed.Related Issues/PRs
This work is done as a part of #518, and implements this decision to remove all
*_devauto
functions that construct tensors without the need to explicitly specify a device.Changes
All the tests, documentation examples etc. were updated.
I've also spotted and fixed a few remaining places that would still use a default device implicitly:
one_hot
, parameters inBatchNorm
etc.Sadly, one aspect I left untouched: de-serialization. It relies on this trait
And currently I don't have a clear idea how to change it in a way that would make a de-serialization to an explicit device possible, while still remaining convenient for the
#[derive(Record)]
macro and non-tensor types likeVec
.So for now de-serialization is still going to be done on a default device first.
Testing
On my machine it passed all the tests I know of.