fix: removing delay and nla bugs, small all around fixes#459
fix: removing delay and nla bugs, small all around fixes#459MartinuzziFrancesco wants to merge 1 commit into
Conversation
Saswatsusmoy
left a comment
Saswatsusmoy
left a comment
There was a problem hiding this comment.
Read through this carefully. The InputDelayESN/DelayESN one looks like it had been silently broken since the input-delay split, since the constructor tests never ran a forward pass — the newst.input_delay.clock == n_steps assertion is a useful regression guard. Left a few questions inline, all curiosity, nothing blocking.
| # :readout)`, so these models need their own parameter/state setup and their | ||
| # own `_partial_apply`. The generic forward call and `collectstates` both route | ||
| # through `_partial_apply`, so overriding it here is enough to make them work. | ||
| const _InputDelayedESN = Union{InputDelayESN, DelayESN} |
There was a problem hiding this comment.
Curious about one thing: since AbstractReservoirComputer{Fields} already encodes the field-name tuple, would it make sense down the line to have the generic initialparameters / _partial_apply introspect Fields directly, so any future composite model with extra pre- or post-fields just works? Could be a follow-up — wondering whether you'd considered that direction, or whether there's a reason a per-model override is preferable here.
There was a problem hiding this comment.
that could be an elegant solution yeha, if there's a way to make it generic enough it should solve similar gotchas to this one
| for idx in eachindex(x_old) | ||
| if firstindex(x_old) < idx < lastindex(x_old) && isodd(idx) | ||
| x_new[idx, :] .= x_old[idx - 1, :] .* x_old[idx - 2, :] | ||
| if firstindex(x_old) < idx && isodd(idx) |
There was a problem hiding this comment.
When firstindex(x_old) > 1 (e.g. an OffsetArray), this would start applying at idx = firstindex + 1 if odd. Is that the intended semantics for offset inputs, or is NLAT2 effectively assumed to receive standard 1-based vectors? Probably never comes up in practice but I noticed it.
There was a problem hiding this comment.
This isn't the only algo that would have issues with nonstandard arrays. It's in the plans to make a larger check for this kind of issues
| data = rand(rng, Float32, in_dims, n_steps) | ||
| target = rand(rng, Float32, out_dims, n_steps) | ||
|
|
||
| idesn = InputDelayESN(in_dims, res_dims, out_dims; num_delays = num_delays) |
There was a problem hiding this comment.
None of the new round-trip tests touch stride > 1. The fix itself isn't stride-sensitive (stride only changes what the buffer keeps), but I wondered if a quick stride=2 smoke test might be worth adding here just to lock the end-to-end behavior down. Or do you think the current coverage is enough?
There was a problem hiding this comment.
yeha that would be a good idea, I'll add it
2 participants
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.