remove some unused Conv constructors #1394
Conversation
|
To be clear, the constructors this deletes are the all-keyword ones. Keeping |
CarloLucibello
force-pushed
the
convt3
branch
from
9d885a7 to
1c75b5f
Compare
|
@DhairyaLGandhi this is waiting for approval |
|
This doesn't remove any of the Dense constructors afaict so could update the news entry, and changes style which I would prefer to keep as the original. Other than than, are we gaining much by removing the constructors? |
no big deal, just some polishing and consistency improvement |
src/deprecations.jl
Outdated
| @deprecate Conv(; weight, bias, σ=identity, kws...) Conv(weight, bias, σ; kws...) | ||
| @deprecate ConvTranspose(; weight, bias, σ=identity, kws...) ConvTranspose(weight, bias, σ; kws...) | ||
| @deprecate DepthwiseConv(; weight, bias, σ=identity, kws...) DepthwiseConv(weight, bias, σ; kws...) | ||
|
|
src/layers/basic.jl
Outdated
| function Dense(in::Integer, out::Integer, σ = identity; | ||
| initW = glorot_uniform, initb = zeros, bias=true) | ||
| function Dense(in::Integer, out::Integer, σ=identity; | ||
| initW=glorot_uniform, initb=zeros, bias=true) |
There was a problem hiding this comment.
changes style - please use the consistent styling for all of Flux/ Zygote. I can appreciate having personal preferences, but I'd prefer to remain with how we have developed it thus far
There was a problem hiding this comment.
Notice that the spacing was inconsistent before. And that no blanks around = is not just my personal preference, it is widely adopted standard, see julia Base or https://github.com/invenia/BlueStyle#whitespace. I'd like to align our codebase to that
There was a problem hiding this comment.
Even if it's not personal preference, it's unwieldy to change these things around if we have an established pattern we use.
There was a problem hiding this comment.
Unless there's any real reason to do so, we can move ahead with the established pattern and revisit if we ever need to change it, and do it consistently then
There was a problem hiding this comment.
It's not really clear there is an established pattern. at present, there isn't much consistence, see the Losses module, the dropout function, normalize,....
There was a problem hiding this comment.
We should fix it there as well, in that case.
|
@DhairyaLGandhi are we good here? |
|
bump |
|
Seems like we either need to fix the docs or these constructors were actually used |
CarloLucibello
force-pushed
the
convt3
branch
2 times, most recently
from
39a6a16 to
c82dfc5
Compare
|
I fixed some rebase mess I created, squashed the commits, fixed documentation and deprecation issues. |
|
@DhairyaLGandhi now should be ready ready :) |
|
@DhairyaLGandhi bump |
|
bors r+ |
1394: remove some unused Conv constructors r=DhairyaLGandhi a=CarloLucibello I think these constructors can go, I suspect no one is using them and they weren't even tested. Docstrings need some love, I won't do it here in order to avoid conflicts with #1391 As a general rule, we consistently provide `Layer(W, b)` constructors, so no need to also have `Layer(; weight=W, bias=b)` for an arbitrary subset of the layers. ### PR Checklist - [ ] Tests are added - [x] Entry in NEWS.md - [ ] Documentation, if applicable - [ ] Final review from `@dhairyagandhi96` (for API changes). Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
|
@DhairyaLGandhi needs a review approval, otherwise bors won't be able to merge given current github's settings |
CarloLucibello
force-pushed
the
convt3
branch
from
c82dfc5 to
e459fe3
Compare
|
rebased |
|
bors r+ |
|
Build succeeded! And happy new year! 🎉 |
|
Ahahh ah, thank you bors |
I think these constructors can go, I suspect no one is using them and they weren't even tested.
Docstrings need some love, I won't do it here in order to avoid conflicts with #1391
As a general rule, we consistently provide
Layer(W, b)constructors, so no need to also haveLayer(; weight=W, bias=b)for an arbitrary subset of the layers.PR Checklist
@dhairyagandhi96(for API changes).