-
-
Notifications
You must be signed in to change notification settings - Fork 43
Feat: Add EIESNAdditive (Model 2) with tests and docs #366
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
Feat: Add EIESNAdditive (Model 2) with tests and docs #366
Conversation
|
Nice, feels like it's almost ready! maybe change the name to |
|
Done. |
|
I would add the bias, one for each activation function. Also note that I transcribed the equations wrong in the issue it seems, eq 5 in the paper indicates that g is a nonlinear function non not a constant. Additionally, I would make the activation also take an array/tuple as input, so the use can either use the same function for both the ones that now are tanh or use different ones. you can add a check similar to here |
|
you can just add one |
src/layers/eiesn_additive_cell.jl
Outdated
| state is not provided. | ||
| """ | ||
|
|
||
| @concrete struct EIESNAdditiveCell <: AbstractReservoirRecurrentCell |
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.
| @concrete struct EIESNAdditiveCell <: AbstractReservoirRecurrentCell | |
| @concrete struct EIESNAdditiveCell <: AbstractEchoStateNetworkCell |
src/layers/eiesn_additive_cell.jl
Outdated
| function initialparameters(rng::AbstractRNG, cell::EIESNAdditiveCell) | ||
| ps = ( | ||
| input_matrix = cell.init_input(rng, cell.out_dims, cell.in_dims), | ||
| reservoir_matrix = cell.init_reservoir(rng, cell.out_dims, cell.out_dims), | ||
| ) | ||
| return ps | ||
| end | ||
|
|
||
| function initialstates(rng::AbstractRNG, cell::EIESNAdditiveCell) | ||
| return (rng = sample_replicate(rng),) | ||
| end | ||
|
|
||
| function (cell::EIESNAdditiveCell)(inp::AbstractArray, ps, st::NamedTuple) | ||
| rng = replicate(st.rng) | ||
| hidden_state = init_hidden_state(rng, cell, inp) | ||
| return cell((inp, (hidden_state,)), ps, merge(st, (; rng))) | ||
| end |
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.
if you subtybe the struct EIESNAdditiveCell <: AbstractEchoStateNetworkCell these can be removed
src/layers/additive_eiesn_cell.jl
Outdated
| - `rng`: Replicated RNG used to initialize the hidden state when an external | ||
| state is not provided. | ||
| """ | ||
|
|
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.
delete the space to make docs build
src/models/additive_eiesn.jl
Outdated
| - `states_modifiers` — a `Tuple` with states for each modifier layer. | ||
| - `readout` — states for [`LinearReadout`](@ref). | ||
| """ | ||
|
|
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.
delete space to make docs build
src/layers/additive_eiesn_cell.jl
Outdated
| win = ps.input_matrix | ||
| A = ps.reservoir_matrix |
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.
use directly the ps., it doesn't save too much space doing this
…cies and add biases
…it from AbstractEchoStateNetworkCell
|
I would say that we are there, just a couple of finishing touches and I can merge this. Feel free to also up the version in the toml so we can register it as soon as we merge it |
|
Thanks for the feedback. Regarding the recurrent part, I noticed that the EIESN formulation requires scaling the matrix product before adding the bias: |
|
that's a good point. Since the scaling is just for the reservoir matrix, and we just have a single bias, you could circumvent that by summing the bias to the input matrix instead, so something like win_inp = dense_bias(ps.input_matrix, inp, bias)
w_state = dense_bias(ps.reservoir_matrix, hidden_state, nothing)
# scale w_state here |
|
That's a great observation regarding associativity, and it would work ideally for a standard ESN.
|
|
I see, that blocks a lot of potential solutions. Then I think your implementation is the right approach, there's probably less overhead for the ifs that there would be to compute the same matmul twice. Just up the version, and make sure that the docstrings also do not go over 92 cols and we can merge this! |
…ersion to 0.12.13
|
Thanks for the feedback! |
This PR implements the EIESN Model 2 (Additive Input) variant as discussed in Issue #353.
In this model, the input is added linearly to the reservoir state after the non-linear activation, rather than being passed through the activation function along with the recurrent part.
Changes:
EIESNAdditiveCellstruct insrc/layers/eiesn_additive_cell.jl.EIESNAdditivemodel wrapper insrc/models/eiesn_additive.jl.input_scaleparameter (representinglayers.mdandmodels.md.ReservoirComputing.jl.Checklist
Additional context
Ref: #353.
This complements the previously merged Model 1 implementation.