search

jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
53 stars 52 forks source link

clean up handling of external 'input' feeds #104

Closed cjayb closed 3 years ago

cjayb commented 4 years ago

While working on #103 (after #98 broke the alpha example) we got hit by inconsistent handling of 'common' and 'unique' (renamed in #103) parameter sets (see discussion there for context).

The (new) test_feed.py:test_extfeed() illustrates the confusion: Poisson and Gaussian input parameters are always created by create_pext, but the 'common' ones aren't.

Variable naming (git grep p_ext) is quite poor, but rather than fiddling with names, it seems time to refactor the parameter handling process.

I suppose we're talking about an internal refactoring, where the API is less important than code clarity? Users shouldn't need to know anything about how the parameters are handled internally, right? The statefulness of the ExtFeed object is currently not used, so is it time to re-write the methods as functions? Or is there a higher meaning that's not obvious?

Some points to discuss here:

jasmainak commented 4 years ago

Yes, I would start by rewriting the ExtFeed as functions. I think that's a good next step. You shouldn't pass the params dictionary into all methods but each function should have a documented set of arguments it accepts

cjayb commented 3 years ago

@jasmainak @rythorpe How do you feel about this issue? I've been off the grid for a long while, but plan to join on Monday for the sprint. If you think I might be of more use elsewhere, please let me know. I notice there are several refactoring issues that I might try to throw myself on.

Edit: you're doing a lot of work on Cells and Networks---perhaps those are a better place to continue from, and see if the handling of these "special" cell types falls out of that work?

jasmainak commented 3 years ago

I think you should definitely pair up with @rythorpe for the sprint. I was suggesting #169 to @rythorpe but open to other ideas. Just make sure to pick something that involves some coding (not just discussion) and that you are determined to finish :) I am wary of the fact that coding sprints often lead to more open issues and questions than answers (also useful but need some balance).

Re-structuring/validating param files might be helpful too but perhaps not as urgent.

perhaps those are a better place to continue from, and see if the handling of these "special" cell types falls out of that work?

Do you mean the external inputs? Maybe thinking along the lines of "what would it take for a user to provide an arbitrary input (not necessarily Gaussian or Poisson) and connect it as they like" would be useful. What kind of refactoring would enable that?

rythorpe commented 3 years ago

A useful place to start would be to refactor ExtFeed so that the object corresponds to one, single feed source (i.e., instead of a collection of feeds). Then, a function in the feed module would be callable by the user in order to instantiate each feed and return a nested list where the outer list contains feed collections associated with e.g. evdist1, evprox2, etc. This list can then be passed into the Network constructor. The main idea here is that every feed instance should contain a few attributes:

rythorpe commented 3 years ago

@cjayb I'm down to try tackling this with you for the coding sprint if you're interested. Either that or we can work on #169 and maybe #138 if we have time.

jasmainak commented 3 years ago

shouldn't gbar_ampa and gbar_nmda etc belong to the Network class? You can specify it when connecting the feed.

What would be ideal is if you can provide the spike times to the feed class. These should be created by separate functions which take in parameters such as mean, std etc.

rythorpe commented 3 years ago

shouldn't gbar_ampa and gbar_nmda etc belong to the Network class? You can specify it when connecting the feed.

Possibly, however it might be nice to have a one-stop-shop for everything related to a given feed source. Right now, we do this by passing around the Params object or p_common/p_unique which is a mess. If we want to make the feed object encode timing and feed type info only, that is cool too though. Any reason for or against the latter case?

What would be ideal is if you can provide the spike times to the feed class. These should be created by separate functions which take in parameters such as mean, std etc.

Yes, this is what I was imagining since the main purpose of the future Feed object will be to succinctly store feed-related information to be accessed by Network.

jasmainak commented 3 years ago

Right now, we do this by passing around the Params object or p_common/p_unique which is a mess.

I agree this should go away.

Any reason for or against the latter case?

nopes, but I don't see why you would need to pass around the Params object? You specify the gbar_ampa and gbar_nmda directly in the Python script and it gets added in the net.ncs / net.connections (for later inspection). So we bypass p_common/p_unique object completely.

rythorpe commented 3 years ago

nopes, but I don't see why you would need to pass around the Params object? You specify the gbar_ampa and gbar_nmda directly in the Python script and it gets added in the net.ncs / net.connections (for later inspection). So we bypass p_common/p_unique object completely.

Oh I see. So avoid any reference to the Params object until it gets passed into Network?

jasmainak commented 3 years ago

yep exactly! Basically, we keep the params object mostly for "creating a network from scratch" or to "share a network", but for modifying the network, we need an API that does not depend on it.

cjayb commented 3 years ago

I'd love to help get rid of passing around params dicts! I'll read up on changes in/around ExtFeed since I looked at it. #138 and #169 look like they might indeed follow nicely. #170 is also a worthy cause for the near future...

cjayb commented 3 years ago

but for modifying the network, we need an API that does not depend on it.

Could you unpack that for me a little more, I'm not sure I get it?

Also, after some perusing of your recent impressive progress, I'm thinking starting at #164, as it seems to contain a lot of goodies we should be sure to acknowledge; WDYT?

Let me see if I've understood the above:

jasmainak commented 3 years ago

I'm thinking starting at #164

totally agree. I was hoping that we could merge it before Monday but I got stuck on some MPI stuff and I'm not quite sure how to fix it. Hopefully @blakecaldwell can be our savior. If it's not merged, you could probably start there to avoid a less painful rebase. Or we could merge it regardless with the hope that the MPI fix does not touch too many lines of code and can be done on Monday.

Ummm ... I'm thinking that someone who uses Python should be able to write the following lines of code:

from hnn_core.feed import Feed, spike_times_poisson

spike_times = spike_times_poisson(...)
feed = Feed(name='rhythmic', spike_times=spike_times, gid=gid)
nc_dict = {'A_delay': 1, 'A_weight': 1e-5, 'threshold': 0.5, 'lamtha': 20}
net.connect_celltypes('rhythmic', 'L2Pyr', loc='proximal', receptor='nmda', nc_dict)

If you want to remove an already existing connection, we can have a method to remove that safely:

net.disconnect_celltypes('rhythmic, 'L2Pyr', 'ampa')

Did I miss anything in my conceptualization? (my understanding of feeds is still a bit rudimentary)

rythorpe commented 3 years ago 
from hnn_core.feed import Feed, spike_times_poisson

spike_times = spike_times_poisson(...)
feed = Feed(name='rhythmic', spike_times=spike_times, gid=gid)
nc_dict = {'A_delay': 1, 'A_weight': 1e-5, 'threshold': 0.5, 'lamtha': 20}
net.connect_celltypes('rhythmic', 'L2Pyr', loc='proximal', receptor='nmda', nc_dict)

Idk if we can specify a feed's gid prior to instantiating the network since unique feeds supply an _ArtificialCell to each cell in the network. gid assignment probably needs to be handled entirely within Network.

jasmainak commented 3 years ago

humm ... there is some thinking to be done there. Maybe:

feed = Feed(name='rhythmic', spike_times=spike_times, unique=True)

and gid assignment happens inside the object? I don't know. Related issue is #128

anyhow right now we are not exposing this to the user, this is just the internal API so to say. Because the NetworkBuilder object is not accessible to the user. That's fine, we got to tackle one thing at a time ... I suggest cleaning up things internally such that they can be exposed to the user in the near future.

jasmainak commented 3 years ago

For me, separating out the spike_times out of a mangled object into separate functions is kind of high priority. Once it's done, we can more easily add other types of inputs (e.g., tonic).