[Breaking] Refactor Backend Names #904
Conversation
|
The description lacks mention of the fundamental rules or principles being followed. Please summarize all changes in terms of rules or principles, so others can easily follow and review your changes. This request stems from the fact that many people are accustomed to the previous convention, so adapting to the new changes will require a shift in both mindset and the code/APIs. |
Agreed! I'm still working on it after an earlier self-review. More descriptions and changes are expected to better follow the Rust naming conventions! |
There was a problem hiding this comment.
It's especially great when long lines like ADBackendDecorator<WgpuBackend<... become Autodiff<Wgpu<...
I also like the fact that the backend word is now reserved for traits only. It's very wordy to say that NdArrayBackend implements Backend, when NdArray implements Backend is enough
| @@ -420,8 +419,7 @@ operation nodes. | |||
| The only remaining part is to implement our autodiff-decorated backend trait for our WGPUBackend. | |||
burn-core/src/lib.rs
Outdated
|
|
||
| #[cfg(feature = "std")] | ||
| #[cfg(test)] | ||
| pub type TestADBackend = burn_autodiff::ADBackendDecorator<TestBackend>; | ||
| pub type TestADBackend = burn_autodiff::Autodiff<TestBackend>; |
There was a problem hiding this comment.
I think we aim at changing every mention of 'AD'?
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
==========================================
- Coverage 86.50% 86.49% -0.01%
==========================================
Files 471 471
Lines 44142 44217 +75
==========================================
+ Hits 38183 38244 +61
- Misses 5959 5973 +14
☔ View full report in Codecov by Sentry. |
This refactor aims to improve the clarity of Burn when selecting a backend. Also, it aims to make it easier to create new backends without having to deal with unreadable associative types through the use of type aliases (
FloatTensor,IntTensor,BoolTensor,FloatElem,IntElem, andDevice).There are breaking changes, but only for the naming. The type aliases don't break anything but simplify the process of creating new backends!
Also, some other changes are made to be more in line with the Rust way of naming things, namely having
UpperCamelCasefor acronyms and contractions, as well as for types.ADBackend=>AutodiffBackendADModule=>AutodiffModuleADTensor=>AutodiffTensorTestADBackend=>TestAutodiffBackendTestADTensor=>TestAutodiffTensorADBackendDecorator=>AutodiffDecoratorOne of the main changes for backend names is to remove the
Backendsuffix, since the types are now available throughburn::backend. Also, I think it's way cleaner to haveAutodiff<LibTorch>than to haveADBackendDecorator<TchBackend>. TheLibTorchchange is also motivated by better clarity. Yes, we usetchfor the bindings, but the most interesting part of the backend isn't the binding, but the code that is used!I removed the re-exported backend type alias such as
WgpuAutodiffBackendsince now the name of the autodiff backend isn't as obscure as before. The biggest benefit is that users will have access to the actual documentation of the backend they use instead of the small doc on top of each alias.The autodiff backend name
ADBackendDecoratorwas bothering me for some time. Including the pattern in the name isn't something really bad in some cases, but we don't add amacrosuffix to all macros orderiveto allDerive, so I think it's better to emphasize the actual behavior that is added instead of the pattern used. I think it's more appropriate to describe the pattern in the documentation.