neurolib-dev/neurolib

New `Stimulus` class wrong shape of output

caglorithm opened this issue · 6 comments

image

Shape of old function was (time, ). Shape of new class is (time, space) but should be (space, time) or only (time, ) if space.ndim == 1.

Thanks to Ben for reporting.

so, the short answer is: it's correct (for MultiModel).. for core neurolib models, you should always use to_ model() and not as_array()

long answer: all stimuli and the classes behind them were created for MultiModel only at first... the idea was that as_cubic_splines() would be necessary for jitcdde backend with adaptive dt, and as_array() would be for numba backend.. numba backend currently expects the model input to be as (time, space) even when space == 1...

what to do:

  • leave it as is, core neurolib would use to_model which returns actually (space, time).. the example notebook on this uses only to_model() so the users "should know".. (yes, I need to write the proper documentation ;) )
  • change model input shape that numba backend expects to match it with core neurolib models... then, both to_model() and as_array() would return the same shape as (space, time).. this would not change anything for the user, as basic MultiModel user-facing interface does not require her to mess around with as_array or as_cubic_splines, as this is done automatically on MultiModel.run()

Uhhm, then the problem is deeper than I expected.

We are using the (space, time) convention everywhere and it should have been the same for multimodel as well. I didn't notice this when I did the review. That means that we have to also fix multimodel as well as the output of as_array()...

yes, but that's work for like 5minutes... the question is, however, where to do it.. My suggestion is to do it inside PR #162 which introduced SummedInput and ConcatendatedInput.. then, I'll rebase PR #166

Multimodel will fail after fixing it I suppose? I would suggest merging #162 (just to keep this one clean) and then opening a new PR with a fix, then we fix #166 after everything is done?

Multimodel will fail after fixing it I suppose?

how do you mean? the tests? or when people use MultiModel? It is not a breaking change, when people use MultiModel via its wrapper they do not create these inputs by themselves, so nothing will change for anyone... the changes would be inside string function that is translated to numba and the shape that as_array() function of stimuli returns

I would suggest merging #162

makes sense, I would rebase #162 on current master anyhow, wait for tests and then we can merge

Resolved in #166