Add temporal embedding and ModernNCA model#49
Conversation
|
@jeremiedb Initially I thought I had to refactor the doc-strings matching julia convention and PR to the Are these cleanups from your side or just incomplete syncs ? , I am confused on this. |
|
Sorry about the sync confusion, |
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
42fcf36 to
fa2bd41
Compare
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
|
@jeremiedb Please take a look when time permits. |
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
|
The Embedding layer still appear heavy at first sight. At least, I think that the docs for the Embedding layer should provide a more explicit description of the design; how one can either provide an embeding config or a dict. NeuroTabModels.jl/src/models/embeddings/config.jl Lines 291 to 292 in ebeda97 Also, why not have Could the the dispatch for For the I understand the current general design where there's no full independency between the embedding and associated architecture adds a layers of complexity. And that the not having full info from the learner (NeuroTabRegressor/Classifier) to build the chain (in cases like piecewise where actual data needs to be seen, or more geenrally for nfeats) also prevent a lightweight implementation. I think this PR made good progress in best accomodating these constraints. I'd still see a need to clear up the few elements raised above before being in a release state. Longer term; I'll look to revisit the design such as to make a convention for the chain to be fully derivable from from the learner. |
| EmbeddingLayer(; num=nothing, temp=nothing) = EmbeddingLayer(num, temp) | ||
| EmbeddingLayer(num::AbstractNumericalEmbedding; temp=nothing) = EmbeddingLayer(num, temp) | ||
|
|
||
| const _NUM_EMBEDDING_TYPES = Dict{Symbol,Union{Type,Nothing}}( |
There was a problem hiding this comment.
In relation to the previous general comment: could we make the EmbeddingLayer defaults to a IdentityEmbedding? So there cannot be a "Nothing" element for it. Then, if there's a temporal embedding (or eventual cat embedding as well), then the Embedding layer performs a vcat of these parallel / branch embedding layers: embed = vcat(numericalembedding(x), temporalembedding(x))
…anch, kwarg constructors Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
jeremiedb
left a comment
There was a problem hiding this comment.
Looks good!
With these latest embedding refactor, I think the PR is in good shape.
Just a little tweak for "testmode".
Also, please add an embedding page to the docs.
| """ | ||
| function embedding_width(layer, x, rng::AbstractRNG) | ||
| ps, st = LuxCore.setup(rng, layer) | ||
| y, _ = layer(x, ps, st) |
There was a problem hiding this comment.
add st = LuxCore.testmode(st), otherwise returns warning of running state in train mode without gradient tracking
There was a problem hiding this comment.
@AdityaPandeyCN could you look at these small remaining items prior to merging this PR and closing the NeuroTabModels story?
There was a problem hiding this comment.
Addressed them.
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
This PR adds temporal embedding feature and ModernNCA model. It also aims to improve docstrings following julia convention.