[MRG] refactoring of external feed class #108

jasmainak · 2020-06-17T21:10:44Z

No description provided.

rythorpe · 2020-06-18T17:05:12Z

@jasmainak I'm going to work on adding the args and corresponding parameters to the Rhythmic and Evoked class methods. Am I clear to push for the next little bit?

jasmainak · 2020-06-18T17:07:31Z

yes go for it! Let me know when you're done

rythorpe · 2020-06-18T17:47:28Z

yes go for it! Let me know when you're done

Done for now! I still need to update the corresponding params but I'll need to save that for later.

jasmainak · 2020-06-18T17:50:29Z

Great!

Btw, do you know if the GUI has a provision for 'normal' vs 'uniform' distribution. I am a little puzzled to see it in the code but not in the GUI?

rythorpe · 2020-06-18T19:23:37Z

Great!

Btw, do you know if the GUI has a provision for 'normal' vs 'uniform' distribution. I am a little puzzled to see it in the code but not in the GUI?

You're right, I don't see the 'uniform' burst distribution referenced anywhere in the GUI or mentioned in the tutorials. I'm guessing this is due to lack of a use-case. Physiologically speaking, I can't think of a situation where a burst event would ever occur with sharp edges. Even a near-synchronous Poisson process wouldn't contain the edge effects present at the bounds of a uniform (i.e., rectangular) distribution.

jasmainak · 2020-06-18T19:30:10Z

okay sounds good. I ask this because it means we don't have to expose an argument to the functions called distribution (at least for now).

jasmainak · 2020-06-20T17:13:32Z

@rythorpe I'm going to do a pass now. Do you have any commits to push?

rythorpe · 2020-06-20T17:26:58Z

Go for it. I won’t be able to work on it until Monday probably.

jasmainak · 2020-06-20T21:11:22Z

hnn_core/network.py

@@ -124,9 +125,7 @@ def __init__(self, params, n_jobs=1):
        # Global number of external inputs ... automatic counting
        # makes more sense
        # p_unique represent ext inputs that are going to go to each cell
-        self.p_common, self.p_unique = create_pext(self.params,
-                                                   self.params['tstop'])


most of what is in this function needs to go inside Rhythmic and Evoked for backwards compatibility. With the new objects, users should be able to either provide legacy params to pre-populate these objects, or set them manually.

jasmainak · 2020-06-20T21:13:00Z

hnn_core/pyramidal.py

-    def parreceive(self, gid, gid_dict, pos_dict, p_ext):
-        for gid_src, p_src, pos in zip(gid_dict['common'],
-                                       p_ext, pos_dict['common']):
+    def parreceive(self, gids, pos_dict, feeds, threshold):


gid was not used anywhere in the function and gid_dict is very opaque. All it does is provide a range of gids. This should be passed directly instead of passing the entire gid_dict.

jasmainak · 2020-06-20T21:13:10Z

hnn_core/pyramidal.py

+
+        Parameters
+        ----------
+        gids : dict


oops, should be a list

jasmainak · 2020-06-20T21:14:29Z

hnn_core/feed.py

+
+    def set_times(self, tstart, tstop, tstart_dev, burst_freq=10, burst_std=20,
+                  burst_n_spikes=2, n_bursts=10, randomize_tstart=False,
+                  random_state1=None, random_state2=None):


simplified version of the existing function.

jasmainak · 2020-06-20T21:15:07Z

hnn_core/feed.py

+        self.L2_Basket_ampa['g'] = g_basket_ampa
+        self.L2_Basket_ampa['g'] = delay
+        self.L2_Basket_ampa['lamtha'] = 100.
+        pass


Suggested change

pass

jasmainak · 2020-06-20T21:22:22Z

@rythorpe I have to stop for today and I may not be able to get to this tomorrow.

I left some comments. Hopefully it begins to make sense. What we're trying to do is:

discourage users from editing the params object directly but rather through an object oriented interface. This allows you to have proper checks and do things more transparently
cleaning some code along the way (getting rid of dead code for instance) + adding tests
documenting the parameters in the docstrings

jasmainak · 2020-06-22T02:16:02Z

hnn_core/feed.py

+                           burst_freq=params[f'f_input_{loc}'],
+                           burst_std=params[f'f_stdev_{loc}'],
+                           burst_n_spikes=params[f'events_per_cycle_{loc}'],
+                           n_bursts=params[f'repeats_{loc}'])


thinking a bit more @rythorpe I think we should just focus on this method / function and do the other methods in another PR

@jasmainak so just use the new Evoked and Rhythmic classes to initialize and store feed attributes and let the Network class handle setting connections for now?

rythorpe · 2020-06-22T05:04:43Z

hnn_core/network.py

@@ -391,6 +392,9 @@ def _create_all_spike_sources(self):
                pc.cell(gid, self.cells[-1].connect_to_target(
                        None, self.params['threshold']))

+        for feed in self.feeds:


So our goal here is to set spike inputs in their own loop (i.e., separate from looping through self._gid_list)? Why is this necessary?

Also, self.feeds should be a list of both self.feeds_common and self.unique_feeds right?

rythorpe · 2020-06-22T22:27:14Z

@jasmainak Can I push something I've been working on?

jasmainak · 2020-06-22T22:45:10Z

Go for it!

…

On Mon, Jun 22, 2020 at 6:27 PM Ryan Thorpe ***@***.***> wrote: @jasmainak <https://github.com/jasmainak> Can I push something I've been working on? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#108 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADY6FIX4PJCZXX5VB7GTILTRX7LE5ANCNFSM4OA7C2XA> .

jasmainak · 2020-06-23T04:10:57Z

hnn_core/feed.py

-
-    def set_times(self, tstart, tstop, tstart_dev, burst_freq=10, burst_std=20,
-                  burst_n_spikes=2, n_bursts=10, randomize_tstart=False,
-                  random_state1=None, random_state2=None):


not sure what you were attempting but I think we should keep this method as a function. Let's talk more tomorrow. Maybe I'm overlooking some challenge

jasmainak · 2020-06-23T04:20:20Z

hnn_core/network.py

@@ -410,9 +410,18 @@ def _create_all_spike_sources(self):
                            params=self.p_common[p_ind],
                            gid=gid))

+                # Convert event times into nrn vector


I think this should be a function:

def artificial_cell(event_times): # the code below # should maybe go into cell.py return nc event_times = rhythmic_input_times(tstart, tstop, burst_freq, n_burst_spikes, ...) nc = artificial_cell(event_times) pc.cell(gid, nc)

jasmainak · 2020-06-23T05:02:51Z

hnn_core/network.py

-                        self.params['threshold']))
+                event_times = get_rhythmic_times(tstart=params['t0'],
+                                                 tstop=params['T'],
+                                                 ...)  # TODO: fill the rest


@rythorpe I probably broke your code :) But I think I am getting a clearer and clearer picture now. I made a skeleton/sketch of what we want to accomplish in this PR. Please take a look and we'll discuss tomorrow. We really should explicitly pass parameters to make the code more transparent. If you get the idea already, feel free to pull the latest commit and continue on top of it. Let's catch up tomorrow in any case!

rythorpe · 2020-07-03T05:49:01Z

@jasmainak We should be clear to move forward with the PR now. At some point we should work on adding more tests for the cell module.

hnn_core/cell.py

hnn_core/feed.py

jasmainak · 2020-07-03T06:01:21Z

hnn_core/network.py

@@ -155,6 +156,9 @@ def __init__(self, params, n_jobs=1):
        self._gid_assign()
        # create cells (and create self.origin in create_cells_pyr())
        self.cells = []
+        # list of artificial cells for storing external feed sources


rather than this comment, I would prefer a more informative comment which explains why you need to create the list in the first place and how it needs to be a list so that the netcon doesn't deactivate

Good point!

hnn_core/cell.py

jasmainak · 2020-07-03T06:02:36Z

hnn_core/cell.py

+    Parameters
+    ----------
+    event_times : list
+        Spike times associated with a given gid


where is the gid?

The gid does not need to specified/known within the scope of this instance, however, I thought that the user should be aware that each ArtificialCell instance is created for a specific gid in the Network class.

hnn_core/network.py

mod/vecevent.mod

jasmainak · 2020-07-03T06:07:53Z

hnn_core/feed.py

@@ -5,7 +5,7 @@
 #          Christopher Bailey <bailey.cj@gmail.com>

 import numpy as np
-from neuron import h
+# from .utils import check_random_state


this PR was originally meant to clean up ExtFeed object. Do you still want to do this in the PR or should we delegate this to another PR? If we do this in another PR, let's delete the above line and the corresponding function.

Yeah, let's delegate this to another PR.

hnn_core/cell.py

rythorpe · 2020-07-06T18:15:57Z

@jasmainak I'm considering making the following changes:

interactive rebase to delete the changes to vecevent.mod in fc60b7f
deleting the check_random_state code in a new commit to retain history
additional commits to address your other concerns mentioned above

Do these sound good to you? If so, do you have any commits to push before I work on this?

jasmainak · 2020-07-06T19:37:35Z

@rythorpe that sounds like the right sequence of steps!

jasmainak · 2020-07-07T02:21:29Z

hnn_core/network.py

+                unique_feed = ExtFeed(feed_type=src_type,
+                                      target_cell_type=target_cell_type,
+                                      params=self.p_unique[src_type],
+                                      gid=gid)
+                self._artificial_cells.append(
+                    ArtificialCell(unique_feed.event_times,
+                                   self.params['threshold']))
+                pc.cell(gid, self._artificial_cells[-1].nrn_netcon)


can we rename _artificial_cells to be _feed_cells to make it clear that these cells relate to the external feed?

Now we have this incongruity that the class ExtFeed does not really create a "feed" but just gives us event_times. I guess it will be fixed once we separate the class properly

Sure, we can do that. So maybe the feed module will eventually contain a single function, callable by the Network class, that creates instances of ExtFeed and ArtificialCell to be passed into pc.cell()?

right, but for command-line interface, I think we need to give user the ability to create feed objects outside of the Network class and then the Network class will simply create the _feed_cells from these objects.

hnn_core/cell.py

hnn_core/tests/test_network.py

jasmainak · 2020-07-07T03:54:28Z

hnn_core/tests/test_feed.py

-            assert ei.cell_type is None  # artificial cell
-            assert hasattr(ei, 'nrn_eventvec')
-            assert hasattr(ei, 'nrn_vecstim')
-            assert ei.nrn_eventvec.hname().startswith('Vector')
-            assert hasattr(ei.nrn_vecstim, 'play')


@rythorpe I think you can have these tests for the new object now?

hnn_core/feed.py

jasmainak · 2020-07-07T03:58:41Z

hnn_core/cell.py

@@ -14,6 +14,42 @@
 # Units for gbar: S/cm^2


+class _ArtificialCell:


we need unit tests for the new object!

jasmainak · 2020-07-07T03:59:48Z

Otherwise LGTM. Let's try to merge this PR soonish so we can move on with life :)

rythorpe · 2020-07-07T21:21:22Z

@jasmainak check out the test I added for the _ArtificialCell class. If we're good there, I'm happy with merging and moving on

jasmainak · 2020-07-08T02:19:58Z

hnn_core/tests/test_cell.py

@@ -16,3 +17,16 @@ def test_dipole():
    with Network(params) as net:
        net.build()
        net.cells[0].plot_voltage()
+
+
+def test_artificial_cell():


docstring is missing. It's displayed during test

jasmainak · 2020-07-08T02:25:03Z

@rythorpe please set the PR to [MRG] once it's good to go for you

codecov-commenter · 2020-07-08T02:49:35Z

Codecov Report

Merging #108 into master will increase coverage by 0.15%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   89.92%   90.08%   +0.15%     
==========================================
  Files          17       17              
  Lines        1708     1704       -4     
==========================================
- Hits         1536     1535       -1     
+ Misses        172      169       -3

Impacted Files	Coverage Δ
hnn_core/feed.py	`65.18% <50.00%> (-0.81%)`	⬇️
hnn_core/cell.py	`86.60% <100.00%> (+0.53%)`	⬆️
hnn_core/network.py	`87.33% <100.00%> (ø)`
hnn_core/tests/test_cell.py	`100.00% <100.00%> (ø)`
hnn_core/tests/test_feed.py	`100.00% <100.00%> (ø)`
hnn_core/tests/test_network.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 015124c...61857ec. Read the comment docs.

rythorpe · 2020-07-08T02:57:28Z

@rythorpe please set the PR to [MRG] once it's good to go for you

Just for reference, what does MRG stand for?

jasmainak · 2020-07-08T04:10:33Z

[MRG] = merge
[WIP] = work in progress

these are two common titles used for PRs :) In the days when github didn't have the approve button folks would also do [MRG +1] and [MRG +2] and then merge the PR once it's [MRG +2]

jasmainak · 2020-07-08T04:11:59Z

whatever happened to Travis CI?

jasmainak · 2020-07-10T02:36:49Z

@blakecaldwell should we wait for your PR #79 to be merged first before merging this to avoid rebase mess? What do you think? One of the PRs has to do the rebase ... not sure which one is easier

blakecaldwell · 2020-07-11T03:00:32Z

@blakecaldwell should we wait for your PR #79 to be merged first before merging this to avoid rebase mess? What do you think? One of the PRs has to do the rebase ... not sure which one is easier

Thanks for checking. Yeah, that would be nice to push #79 first. After these two, we will have a good handle on the Network and NeuronNetwork refactoring that remains.

jasmainak · 2020-07-15T01:32:15Z

@rythorpe we will need to rebase this PR now. Up for the challenge? ;-)

rythorpe · 2020-07-15T04:14:33Z

@rythorpe we will need to rebase this PR now. Up for the challenge? ;-)

I need to spend some time wrapping my head around the new structure, so yeah, I'll give it a shot!

add space create methods for Evoked and Rhythmic classes Expanding more params Network class handles nrn feed objects Try artificial cell move to feed blah create ArtificialCell class for initializing external feed sources add docstring and clean code fix test_feed.py fix docstrings and comments in ArtificialCell and Network classes remove check_random_state() from utils module remove feed list attributes in Network class make artificial cell class private add unit test for _ArtificialCell class remove useless return statements from '_create_feed' functions Update hnn_core/feed.py comment Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com> update docstrings in feed tests

jasmainak · 2020-07-22T22:08:19Z

@rythorpe I did a rebase after squash. Can you check if this was done correctly? If not, can you push fixes?

…Network class

rythorpe · 2020-07-24T06:14:02Z

Thanks for the rebase @jasmainak! Aside from the TravisCI issue on the MacOS build job, all tests now pass. I'm ready to move forward whenever you and Travis are.

jasmainak · 2020-07-24T06:55:47Z

okay let's move on. The MacOS build needs to be fixed separately. We should probably pin the NEURON version for now.

thanks a lot for the effort @rythorpe !

jasmainak commented Jun 20, 2020

View reviewed changes

hnn_core/pyramidal.py Outdated

Parameters

----------

gids : dict

Copy link

Collaborator Author

jasmainak Jun 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, should be a list

jasmainak commented Jun 20, 2020

View reviewed changes

jasmainak commented Jun 22, 2020

View reviewed changes

rythorpe reviewed Jun 22, 2020

View reviewed changes

jasmainak force-pushed the refactor_feed branch from 8721db2 to f801832 Compare June 22, 2020 18:44

jasmainak commented Jun 23, 2020

View reviewed changes

jasmainak force-pushed the refactor_feed branch 2 times, most recently from 97da41d to fc60b7f Compare June 24, 2020 05:49

jasmainak commented Jul 3, 2020

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

rythorpe force-pushed the refactor_feed branch from b87fc10 to 9cb4590 Compare July 7, 2020 00:53

jasmainak commented Jul 7, 2020

View reviewed changes

hnn_core/cell.py Outdated Show resolved Hide resolved

jasmainak commented Jul 7, 2020

View reviewed changes

hnn_core/tests/test_network.py Show resolved Hide resolved

jasmainak commented Jul 7, 2020

View reviewed changes

hnn_core/feed.py Outdated Show resolved Hide resolved

jasmainak commented Jul 7, 2020

View reviewed changes

jasmainak commented Jul 8, 2020

View reviewed changes

rythorpe changed the title ~~refactoring of external feed class~~ [MRG] refactoring of external feed class Jul 8, 2020

jasmainak force-pushed the refactor_feed branch from 61857ec to 04e0cd4 Compare July 22, 2020 22:07

rythorpe mentioned this pull request Jul 23, 2020

WIP: Refactor extfeed #88

Closed

rythorpe added 3 commits July 24, 2020 00:50

fix references to Network vs NeuronNetwork attributes

f7e0e9e

remove unnecessary unique_feeds and common_feeds attributes of Neuron…

13dec30

…Network class

remove second import of _ArtificialCell

c95dc23

jasmainak merged commit 4b4d319 into jonescompneurolab:master Jul 24, 2020

blakecaldwell mentioned this pull request Jul 30, 2020

MAINT: build mod files as part of install #127

Merged

		@@ -14,6 +14,42 @@
		# Units for gbar: S/cm^2


		class _ArtificialCell:

[MRG] refactoring of external feed class #108

[MRG] refactoring of external feed class #108

Conversation

jasmainak commented Jun 17, 2020

rythorpe commented Jun 18, 2020

jasmainak commented Jun 18, 2020

rythorpe commented Jun 18, 2020

jasmainak commented Jun 18, 2020

rythorpe commented Jun 18, 2020

jasmainak commented Jun 18, 2020 • edited

jasmainak commented Jun 20, 2020

rythorpe commented Jun 20, 2020

Choose a reason for hiding this comment

jasmainak Jun 20, 2020 • edited

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 Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Jun 22, 2020

jasmainak commented Jun 22, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Jul 3, 2020

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

rythorpe commented Jul 6, 2020

jasmainak commented Jul 6, 2020

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 Jul 7, 2020

rythorpe commented Jul 7, 2020

Choose a reason for hiding this comment

jasmainak commented Jul 8, 2020

codecov-commenter commented Jul 8, 2020

Codecov Report

rythorpe commented Jul 8, 2020

jasmainak commented Jul 8, 2020

jasmainak commented Jul 8, 2020

jasmainak commented Jul 10, 2020

blakecaldwell commented Jul 11, 2020

jasmainak commented Jul 15, 2020

rythorpe commented Jul 15, 2020

jasmainak commented Jul 22, 2020

rythorpe commented Jul 24, 2020

jasmainak commented Jul 24, 2020

jasmainak commented Jun 18, 2020 •

edited

jasmainak Jun 20, 2020 •

edited