MRG: improve feeds #204

jasmainak · 2020-11-12T02:21:38Z

Giving this another shot. To complement efforts by @cjayb

hnn_core/feed.py

codecov-io · 2020-11-12T03:39:59Z

Codecov Report

Merging #204 (9b51bf7) into master (fd545cb) will increase coverage by 1.22%.
The diff coverage is 67.82%.

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
+ Coverage   67.78%   69.00%   +1.22%     
==========================================
  Files          21       21              
  Lines        2213     2168      -45     
==========================================
- Hits         1500     1496       -4     
+ Misses        713      672      -41

Impacted Files	Coverage Δ
hnn_core/feed.py	`49.31% <48.61%> (+5.43%)`	⬆️
hnn_core/__init__.py	`100.00% <100.00%> (ø)`
hnn_core/network.py	`53.08% <100.00%> (-0.39%)`	⬇️
hnn_core/tests/test_feed.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd545cb...9b51bf7. Read the comment docs.

jasmainak · 2020-11-12T20:53:45Z

okay this PR is beginning to get big. I am now planning to add unit tests for each of the different functions I documented. Might get to it next week. But in the meanwhile, would appreciate another set of eyes to look at it (@cjayb @rythorpe @ntolley ?).

The easiest way to review would be to look at the new feed.py file along with the diff. Here is a summary of the main changes:

simplification of seed generation with a _get_prng method.
dismantling of ExtFeed class to feed_event_times function.
unifying truncating event_times > 0 and sorting of event_times in one function
unification of several if-else clauses, for example in the _create_common_input function (previously a method)

I hope this helps a lot in seeing clearly what we should do next with feeds.

cjayb

Apart from a single typo, I find this to be only positive. A lot of the nestedness is gone, even though there remain a lot of idiosyncrasies from "HNN Classic". With #191 we can get all this into Network, where it'll be easier to think about a good API (and tests!) for prng as well. Definite +1 from me!

cjayb · 2020-11-13T16:33:53Z

hnn_core/feed.py

+        The events per cycle. Must be 1 or 2.
+    prng : instance of RandomState
+        The random state.
+    prng : instance of RandomState


cjayb · 2020-11-13T16:37:53Z

hnn_core/feed.py

+        # Two arrays store doublet times
+        t_input_low = t_array - 5
+        t_input_high = t_array + 5
+        # Array with ALL stimulus times for input
+        # np.append concatenates two np arrays
+        t_input = np.append(t_input_low, t_input_high)


So this isn't a comment on this PR, more to the code: Could we get one of the OPs to give us a link to the part of documentation where this is specified?

maybe @rythorpe or @ntolley can help since they use the GUI?

I don't understand. What specifically are you looking for here @cjayb?

The code is now simplified to

return np.append(t_array - 5, t_array + 5)

but doesn't it creep you out that there are undocumented, hard-coded time shifts there? Clearly it's creating spike "doublets", and certainly for a very specific reason. I was just wondering whether anyone still remembers what the reason was?

but doesn't it creep you out that there are undocumented, hard-coded time shifts there?

LOL yes :) Maybe it makes sense for some reason, I don't know ...

I think @rythorpe or @ntolley can throw some light on what this function does and how the spike train is supposed to look like and why. I opened the GUI many aeons ago ...

Perhaps you should restore the comment explaining how this is equivalent to the spikes/burst parameter in GUI?. As for the fixed time shifts, I'm guessing there is a biologic justification for a doublet with a 10 ms ISI.

I see that it was added in a pretty large commit in the GUI.

I think the 10 ms is a somewhat standard approximation for the amount of time between spikes within a thalamic burst. The HNN webset/tutorials don't explicitly address this, however, they do reference this paper that discusses the bursting properties of thalamic drive into cortex.

Ideally we'll define an API in a later PR for creating rhythmic feeds that allows the user to define the intra-burst ISI.

done, I updated the docstring :) A demo of the GUI for these functions would be nice one day, now that we have everything nicely documented in the code.

hnn_core/feed.py

rythorpe · 2020-11-13T22:38:20Z

hnn_core/feed.py

+    prng : instance of RandomState
+        The seed for events assuming a given start time.
+    prng2 : instance of RandomState
+        The seed for start times.


I'm a little confused on the use of these two prng values. Could we clarify them a little in the documentation?

to be honest, I am a bit confused myself. I thought prng2 is used to get the start time jitter but actually it turns out if t0 = -1 then prng is used. See:

hnn-core/hnn_core/feed.py

Lines 225 to 230 in 56f3565

if t0 == -1:

t0 = self.prng.uniform(25., 125.)

# randomize start time based on t0_stdev

elif self.params['t0_stdev'] > 0.0:

# start time uses different prng

t0 = self.prng2.normal(t0, self.params['t0_stdev'])

Nonetheless I clarified in the documentation assuming the intent was actually for the start time for both cases.

It looks like prng2 is only used for rhythmic inputs and defines the jittered start time of the rhythmic burst. Then, prng is used to define the jittered input of single input within that burst. If my understanding is correct, we may want to simplify _get_prng() by elimnating any reference to prng2 and then adding the logic for creating a second random state variable to feed_event_times() only if a rhythmic (i.e., common) input is in the making.

Actually, this will probably get resolved when we work on a more flexible API for creating new feeds so let's ignore this for now.

That makes sense. Note though the current "API". Every function has the last argument as prng. This is how it should be. The user should be able to supply their own prng. It shouldn't be generated opaquely inside a function or class

rythorpe · 2020-11-13T22:44:28Z

hnn_core/feed.py

+    return prng, prng2
+
+
+def feed_event_times(feed_type, target_cell_type, params, gid):


Would this be a good time to start explicitly defining the elements of the params argument for this function?

not sure if it would be a good use of our time to document what is created as an output of create_pext. I think it might be helpful to move create_pext into feed.py so folks can see what parameter corresponds to what. We would avoid duplicating what is in params.py just for documentation ... Let me know if you want me to do that or if you'd rather keep that in params.py

I'm a strong +1 on leaving as-is for this PR. create_pext is a symptom of unwieldy parameter handling, so fixing (or documenting) one or two methods, I think, has lower priority than figuring out what to do about the root cause.

We can wait on this. At some point we need to start dismantling the large param dicts that get passed around and I'm beginning to wonder if the best way to start this process is to begin explicitly defining function arguments where we can.

Very likely so, let's see what we can do with a new feeds (drives) API

+100 on explicitly defining function arguments where we can! It's the best way to avoid opaque code

jasmainak · 2020-11-15T06:49:30Z

Alright, I'm done here. Addressed all the comments and updated tests.

feed.py is massively simplified. Half of the file is now just docstrings. I don't quite get _create_common_input is supposed to do so I didn't test any of the functionality other than checking that errors are raised correctly. I'd leave this checking for another PR. But any help from GUI users to explain the purpose of this would be very helpful :)

The poisson input function is simplified and it uses numpy now.

Before you decide to merge, please make sure that the example plots (in the CircleCI artifacts) look similar to what we see on the current version of the website. Thank a lot for reviewing guys!

jasmainak · 2020-11-16T22:37:07Z

PR rebased :)

cjayb · 2020-11-16T22:43:21Z

Sweet! I'm offline until tomorrow so feel free to merge if you think it's clean enough.

cjayb · 2020-11-17T14:30:56Z

Bringing discussion on net.trial_event_times here @jasmainak :

It should be dict (keys are the src_type) of list (gids) of list (n_trials) of list (n_events) ? I think it will help unify things related to trial jitter etc.

The place look at is NetworkBuilder._create_cells_and_feeds:L404: the for-loop over all gids on this rank. I suppose you could do:

        for gid in self._gid_list:  # L404
            src_type, src_pos, is_cell = self.net._get_src_type_and_pos(gid)
            this_src_event_times = self.net.trial_event_times[src_type]
    ...
        else:
            gid_idx = gid - self.net.gid_ranges[src_type][0]  # still needed
            et = this_src_event_times[self.trial_idx][gid_idx]
...

Want to try to flip it around (to dict of lists)? Perhaps rename net.trial_event_times (back) to net.feed_event_times. NB for single-trial simulations this means that the first list dimension len is 1.

cjayb · 2020-11-17T14:31:41Z

I'm also +1 for a merge.

jasmainak · 2020-11-17T15:47:04Z

hnn_core/feed.py

-        if lamtha > 0.:
-            t_gen = t0 + self._t_wait(lamtha)
-            if t_gen < T:
-                np.append(val_pois, t_gen)


@rythorpe found the source of the discrepancy you noticed in the gamma example. When you use np.append, the return value needs to be val_pois. The array is not modified in-place like a list. So, this is actually a bug in the old code. One spike is missed at the beginning. Should we consider this a bugfix (and document it in whats_new.rst) or should I push a commit to revert to the old behavior?

Oh I see. This is a good find. My vote is to move forward with your current solution as a bug fix and document it in whats_new.rst.

hnn_core/feed.py

jasmainak · 2020-11-17T15:52:58Z

Want to try to flip it around (to dict of lists)?

Let's keep this for another PR. I don't want to increase the size of this PR any further :)

jasmainak · 2020-11-18T04:25:37Z

I updated the whats_new with the bugfix note

rythorpe · 2020-11-18T04:33:51Z

Any other last minute fixes or are we good to rebase and merge?

jasmainak · 2020-11-18T04:34:09Z

Good to merge!

rythorpe · 2020-11-18T04:34:47Z

Done! Thanks @jasmainak!

jasmainak force-pushed the improve_feeds branch from f448dce to 6ccce68 Compare November 12, 2020 02:56

jasmainak commented Nov 12, 2020

View reviewed changes

hnn_core/feed.py Outdated Show resolved Hide resolved

jasmainak force-pushed the improve_feeds branch 2 times, most recently from fea9630 to 64d5040 Compare November 12, 2020 20:19

cjayb approved these changes Nov 13, 2020

View reviewed changes

rythorpe reviewed Nov 13, 2020

View reviewed changes

hnn_core/feed.py Show resolved Hide resolved

rythorpe reviewed Nov 13, 2020

View reviewed changes

hnn_core/feed.py Outdated Show resolved Hide resolved

rythorpe reviewed Nov 13, 2020

View reviewed changes

hnn_core/feed.py Outdated Show resolved Hide resolved

rythorpe reviewed Nov 13, 2020

View reviewed changes

jasmainak force-pushed the improve_feeds branch 3 times, most recently from 2678b97 to 508799a Compare November 15, 2020 05:30

jasmainak changed the title ~~WIP: improve feeds~~ MRG: improve feeds Nov 15, 2020

jasmainak mentioned this pull request Nov 15, 2020

[MRG] Refactor creation of network 'feeds' (to allow new mechanism for adding network drives) #191

Merged

4 tasks

jasmainak added 12 commits November 16, 2020 17:05

WIP: improve feeds

c19b049

WIP: evoked inputs

95f0bfd

Extgauss

c1ef2ba

DOC return value

c0d6386

MAINT: zero_ampa_nmda

f36d051

MAINT: move syn weights check outside

6331444

TST make them happy

0a6b748

MAINT startswith can take tuple

c29c86a

DOC: start documenting common inputs

b7dfeee

MAINT: remove orphan self.attrib

31b8822

MAINT: simplifications to _create_common_input

c9601f7

MAINT: simplify _get_prng

3162267

jasmainak added 11 commits November 16, 2020 17:05

FIX: seed > 0

0029cf6

MAINT: unify sorting and truncating above 0

c5f0626

Cosmit

af334d4

MAINT: dismantle ExtFeed class

8899def

Flake8

ef6c4dd

DOC: fixes

42d63f2

MAINT: no need for two functions

8d2360e

MAINT: no need for sigma > 0

85fe028

MAINT: simplify poisson spike train generation

aecd01f

TST add tests for common inputs

be5cdc0

DOC: zero_ampa_nmda -> target_syn_weights_zero

992a460

jasmainak force-pushed the improve_feeds branch from 9e47d91 to b0d974f Compare November 16, 2020 22:36

jasmainak force-pushed the improve_feeds branch from b0d974f to 5658324 Compare November 16, 2020 22:38

MAINT: rebasing issues

9b51bf7

jasmainak force-pushed the improve_feeds branch from 5658324 to 9b51bf7 Compare November 17, 2020 02:26

rythorpe approved these changes Nov 17, 2020

View reviewed changes

jasmainak commented Nov 17, 2020

View reviewed changes

hnn_core/feed.py Show resolved Hide resolved

DOC: update whats_new and docstring

0d241be

rythorpe merged commit b506c2f into jonescompneurolab:master Nov 18, 2020

rythorpe mentioned this pull request Nov 24, 2020

poisson input causes descrepancy with tutorial jonescompneurolab/hnn#264

Open

	if t0 == -1:
	t0 = self.prng.uniform(25., 125.)
	# randomize start time based on t0_stdev
	elif self.params['t0_stdev'] > 0.0:
	# start time uses different prng
	t0 = self.prng2.normal(t0, self.params['t0_stdev'])

		return prng, prng2


		def feed_event_times(feed_type, target_cell_type, params, gid):

MRG: improve feeds #204

MRG: improve feeds #204

Conversation

jasmainak commented Nov 12, 2020

codecov-io commented Nov 12, 2020 • edited

Codecov Report

jasmainak commented Nov 12, 2020 • edited

cjayb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Nov 15, 2020 • edited

jasmainak commented Nov 16, 2020

cjayb commented Nov 16, 2020

cjayb commented Nov 17, 2020

cjayb commented Nov 17, 2020

jasmainak Nov 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Nov 17, 2020

jasmainak commented Nov 18, 2020

rythorpe commented Nov 18, 2020

jasmainak commented Nov 18, 2020

rythorpe commented Nov 18, 2020

codecov-io commented Nov 12, 2020 •

edited

jasmainak commented Nov 12, 2020 •

edited

jasmainak commented Nov 15, 2020 •

edited

jasmainak Nov 17, 2020 •

edited