Add sbi wrapper class

[akapet00]

Attempt to resolve the issue #44.

The class Inferencer is fully functional and can be tested via hh_sbi.py file in examples.
It supports both multi-round inference and multi-input/output traces.

Even though the class is ready to use, the feature metric is not yet fully supported.
This actual feature metric should be supported by resolving the issue #48.

All additional changes are more or less stylistic improvements.
sbi is added in the requirements file and in the setup.py as the optional dependency.

[mstimberg]

Everything looking 👍 in general. I left a few specific comments, but nothing major. Regarding the features, I'd think I'd do a short-term solution before dealing with #48 (there are some subtleties we have to take into account, e.g. eFEL often needs stimulus times as an additional argument). This solution would be that the train function (and/or Inferencer.__init__) takes a list of functions which each takes as input a 2d-array of traces (samples × inputs) and should give as an output either a 2d array (features × inputs), or a 1d array of shape (inputs, ) (as convenience for scalar features). These will then be flattened and concatenated before handing over to sbi. Note that I did not restrict it to scalar features, I think in some cases it is simpler to write a single function that returns several features instead of asking the user to split them up into multiple functions. The only requirement is that the number of features returned by a function never changes (so something like "spike times" would not be a valid feature, but "min and max membrane potential" would be). In the long run we should certainly check this for a nice error message, but in the short run it is ok if it just fails somewhere in the pipeline with a shape mismatch.

[mstimberg]

I think there was a reason for the autonotebook import (nicer progress bars in notebooks I guess?). Not sure what the best solution is here, you presumably removed it because of the warning?

[akapet00]

Yes, autonotebook looks better but also throws the warning and sometimes fail due to IProgress not found.
Of course, I will change this import as it has been done so far and use tqdm from autonotebook module.

[mstimberg]

Minor nitpick (for here and in many other places): the docstrings are restructured text not markdown, so single backticks are meant to create links to class/function names, double backticks should be used for simple "typewriter" text.

[akapet00]

Oh, right. I will change that 👍

[mstimberg]

You should not need to get the units and divide by them, you can always call np.array around quantities (which will not even perform a copy). In some places you can also get a unit-less version by appending an underscore (this is a Brian convention), e.g. statemonitor.t_ would give you the recorded time points without units.

[mstimberg]

See above comment about units, e.g. here you could simply use get_value().

[mstimberg]

I think it would be good to divide this up a bit further, e.g. have one method to simulate from the priors (or from posteriors from a previous run in the multi-round approach), one method to train the network, and one method to build the posterior. There are several reasons for this: 1) it makes it possible to train a network and then use the trained network on different experimental data (e.g. different cells) 2) it can be useful for debugging/sanity checks (e.g. if half of the simulations from the prior distribution give completely senseless results, it probably makes sense to adapt the prior before starting to train a network and 3) sbi's train() and build_posterior() methods both take several arguments. For advanced users, it would be good if they could provide them (which would be a mess with the current Inferencer.train() method. Having said all that, I think your Inferencer.train() method is a good convenience method for the most common use case, so I'd keep it and just have it call out to the more fine-grained methods .

[akapet00]

Yes, I completely agree.
I was thinking to enable a simple call to train method to do everything for the user, but for more advanced user break this up into a smaller chunks. For example, one method for the actual training of the estimator, one method for building posterior, one method for drawing samples from the posterior, and finally, one method for visual inspection.
Also, with this, we will be able to create easier calls for store and load of the trained posterior, ref to issue #46.

[mstimberg]

Yet another minor thing: for a quick look how the fitting went, it would be good to introduce a Inferencer.generate method (like Fitter.generate) that can then be used to visualize the fitted traces in comparison to the experimental ones (like in the other examples).

[akapet00]

Sure thing :)

[akapet00]

Marcel, thanks for great comments!
We can discuss the issue #48 in greater details in near future.
For now, I'll try to create this list of functions for feature extraction possible.
I guess it would be okay if some of the functions from that list use eFEL if the start and end stimulation times are known a priori?

[mstimberg]

For now, I'll try to create this list of functions for feature extraction possible.
I guess it would be okay if some of the functions from that list use eFEL if the start and end stimulation times are known a priori?

So just to avoid misunderstanding, I meant that the inferencer gets something like features=[fun1, fun2, fun3] as an input, with fun1, etc. being functions mapping traces → features, all independent of eFEL. So from the Inferencer side we would not offer any functions, all functions would be provided by the user. Concretely, the hh_sbi.py example would define a function similar to what is current Inferencer.extract_features (which would be removed from Inferencer) and pass it as an argument for features. In general, the provided functions could certainly be a wrapper around eFEL, but the Inferencer code should (at this stage) do not need to know that eFEL exists, that there is such a thing as a stimulation time, etc.

[akapet00]

In general, the provided functions could certainly be a wrapper around eFEL, but the Inferencer code should (at this stage) do not need to know that eFEL exists, that there is such a thing as a stimulation time, etc.

That's exactly what I was thinking. Anyway, thank you for the clarification!

[akapet00]

Last few commits deal with some minor docstrings issues.
Also train method is renamed to infere. This was done to be able to refactor the initial train method to 3 additional methods. The reason behind this is stated here:

I think it would be good to divide this up a bit further, e.g. have one method to simulate from the priors (or from posteriors from a previous run in the multi-round approach), one method to train the network, and one method to build the posterior. There are several reasons for this: 1) it makes it possible to train a network and then use the trained network on different experimental data (e.g. different cells) 2) it can be useful for debugging/sanity checks (e.g. if half of the simulations from the prior distribution give completely senseless results, it probably makes sense to adapt the prior before starting to train a network and 3) sbi's train() and build_posterior() methods both take several arguments. For advanced users, it would be good if they could provide them (which would be a mess with the current Inferencer.train() method. Having said all that, I think your Inferencer.train() method is a good convenience method for the most common use case, so I'd keep it and just have it call out to the more fine-grained methods .
This also solves #50.

Advanced users can use each of the methods init_inference, train and build_posterior separately to gain complete control over the inference. Utility method 'infere' achieves similar results but does not offer such a powerful control over sbi.

Also, functionality for generating fitted traces by using the inferred parameters is added.

The example is updated according to these changes.

[mstimberg]

Ok, all looks good to me for a merge – I made some minor changes along the way, but nothing really important. We'll need to get into tests and documentation at some point of course, but for now let's continue a bit until the API is settled.

[akapet00]

Great, thanks!