Fix dipole and network (spiking) class write methods #96

rythorpe · 2020-03-27T02:39:21Z

Two changes have been made: 1) dipole.write() has been fixed to save files according to the user-specified name, and 2) network.write_spikes() has been added so that spiking data for selected trials can be saved to a single .txt file. See the attached example (a modified version of the plot_simulate_evoked.py example) and its corresponding output .txt and .png files that demonstrates reconstruction of the simulated spike event plot using the spike .txt files.
demo.zip

jasmainak · 2020-03-27T06:26:34Z

hnn_core/network.py

+        for spike_type,gid_range in self.gid_dict.items():
+            gidtypes[np.in1d(spikegids,gid_range)] = spike_type
+
+        with open(fname,'w') as f:


you should use standard functions whenever possible. In this case, I think you could use np.savetxt

Because I combined data types (i.e., list of floats and list of strings) which don't easily combine into a single array, the most succinct way to handle the formatting was to use low-level I/O functions. I could potentially convert each list to a formatted string, then convert it to a numpy array, and then save it if you think that'd be better?

Note that HNN originally stored spike.txt files along side param.txt files that stored the gid ranges. I tried to retain the HNN format but also combine all the relevant spiking info into a single file.

Since we are proceeding with using hnn-core for HNN. I'd prefer to keep spikes for different trials in separate files, as with HNN. If it's not, I will have to write a function for HNN that reads the single file and then splits into different files. :(

It is currently setup to take an argument (i.e., trial_idx) that specifies which trials are selected to write to the file, so the write function can be looped over to save all trials separately. The reason for this was to mimic the low-level functionality of the dipole.write() function which also only writes one file at a time. Would it be more helpful if we changed both the dipole.write() and network.write_spikes() functions to output all files with the same naming convention as HNN?

Oh, yes. This is where a separate Spike (Spikedata) class would be helpful for parity with Dipole: Spikedata.write() and Spikedata.write_all_trials(). That way you can enforce some standard naming (like HNN for better or worse) and don't have the ambiguous case of naming a file with some, but not all trials.

There's more than one way to do it. Just my thoughts. Thanks for your work on this.

jasmainak · 2020-03-27T06:29:37Z

hnn_core/network.py

+        Parameters
+        ----------
+        fname : str
+            Full path to the output file (.txt)


What motivated the use of a .txt file instead of say an .npz or .mat file?

I just wanted to be consistent with the dipole.write() function and previous HNN I/O convention.

Since testing new hnn-core output against old hnn output will be important for the foreseeable future, I plead that we leave files the same (.txt). We will have test functions to compare array values after loading from the file, but also being able to do a diff from the command line is useful.

jasmainak · 2020-03-27T06:31:30Z

hnn_core/network.py

+            Indices of selected trials. If 'all',
+            all trials are selected.
+        """
+        if trial_idx=='all':


use None. The problem with 'all' is that string comparison may fail if it's not the right case

jasmainak · 2020-03-27T06:32:01Z

hnn_core/network.py

@@ -518,3 +518,32 @@ def plot_spikes(self, ax=None, show=True):
        if show:
            plt.show()
        return ax.get_figure()
+
+    def write_spikes(self, fname, trial_idx='all'):
+        """Write spike times to a file.


how do you plan to read them back in?

Check out the plot_simulate_evoked.py script in the attached demo.zip folder. Are you thinking we should have a built-in read function as well?

HNN has the functionality to take a given parameter file, find the spike output in an expected directory and load it. If this functionality is part of hnn-core, HNN can use it. Otherwise, the function will still be part of HNN.

So are you in favor of adding a read function in hnn-core or would you rather that be handled by an HNN wrapper?

I think you should add a reader if you're going to have a way to write it :) It's also easier to test that way. You can do a round trip and check if you have what you started with.

Since spikes currently exist only as a network class attribute, they either need to be read in through a network class method at/after network initialization or handled as their own object outside of the network class (as with params). In keeping with our desire to make HNN data structures more transparent and tractable (e.g., using .json files for params, json schema for param validation), what do you think of turning spikes into their own object with the following example structure:

{ <gid_type>: { <gid>: [ <spiketime[0]>, <spiketime[1]>, ... ] } }

We could still maintain the .txt file format convention of HNN output for now, but this would make the spike data more intuitive, modular, and readable into a network instance with proper tests and validation.

Humm ... I would keep it simple for now. You can read in a dictionary of lists. The dictionary would contain three keys gid, gid_type and spiketimes. It sounds like this stuff should belong to the _Cell class but I'm sure it's nasty to save it directly and read it back. Let's stick to python built-ins until it becomes apparent if something should go to a separate class.

thinking more, you could probably use a list of NamedTuple. Maybe that simplifies some code, I don't know ... would keep it for another PR.

Yeah, a reader would be helpful. In the future, I like the idea of moving spiketime and spikegids out of the network class. I'm in favor of either keeping things as close to HNN for now or the other extreme of making a new Spike or Spikedata class.

The namedtuple class does look super convenient for this - good call. In the short term, I'll just implement a simple read function that validates gids prior to them overwriting the spikes of a network instance (to be compatible with the ranges in network.gid_dict).

jasmainak · 2020-03-27T06:33:45Z

hnn_core/network.py

+            Full path to the output file (.txt)
+        trial_idx : list of int
+            Indices of selected trials. If 'all',
+            all trials are selected.


can you also document what the file actually looks like?

The file is formatted in three columns where each row is <spike_time>\t<spike_gid>\t<gid_type>. There is an example in the demo.zip folder. Are you looking for a formal description in the function documentation?

yep, a formal description in the function docstring

jasmainak · 2020-03-27T06:34:19Z

hnn_core/network.py

+
+        spiketimes = []
+        spikegids = []
+        for i in trial_idx:


avoid use of single letter variable names when possible. They are hard to Ctrl + F

blakecaldwell · 2020-03-27T13:57:16Z

Thanks @rythorpe!

hnn_core/dipole.py

jasmainak · 2020-03-28T04:32:17Z

@rythorpe also don't forget to update whats_new.rst and api.rst once you have added the reader function.

jasmainak · 2020-03-30T23:52:51Z

hnn_core/network.py

+            Indices of selected trials. If None,
+            all trials are selected.
+        """
+        if trial_idx==None:


Suggested change

if trial_idx==None:

if trial_idx is None:

https://docs.quantifiedcode.com/python-anti-patterns/readability/comparison_to_none.html

jasmainak · 2020-03-30T23:55:37Z

hnn_core/network.py

+            spikegids += self.spikegids[idx]
+
+        gidtypes = np.empty_like(spikegids,dtype='<U36')
+        for spike_type,gid_range in self.gid_dict.items():


you need a PEP8 checker. For some reason Travis didn't run on your branch and PEP8 didn't run. Let me rebase your branch so you see the problem.

That's weird your branch is up to date. @blakecaldwell any idea why Travis is not running? I saw you do some recent experiments on HNN with Travis.

I'm not sure... Do you have this repository on your Travis dashboard? I haven't linked it to my account. I've found the Travis dashboard to have useful hints under the "requests" section. If it fails because of a bad .travis.yml, for example, it will indicate it there (doesn't appear to the case here, though).

I do have it under the dashboard. Maybe @rythorpe just try pushing another commit and it might work ...

rythorpe · 2020-04-02T21:15:32Z

I don't know why my latest push hasn't updated the PR yet, but here's a python script that does three round trip tests for spike and dipole read/write functions. I wasn't sure if any of these were appropriate to add as formal tests.

test_write_read.zip

jasmainak · 2020-04-05T03:19:28Z

@rythorpe do you see an error when pushing? what's the issue?

rythorpe · 2020-04-05T03:59:13Z

@rythorpe do you see an error when pushing? what's the issue?

My latest commit doesn't appear in this thread. Am I missing something?

jasmainak · 2020-04-05T19:43:21Z

@rythorpe that was so strange. I'm guessing it was related to this incident. I fixed it for you now by making a new commit, pushing again, and then removing that commit.

rythorpe · 2020-04-10T03:35:58Z

I logged the following error traceback when trying to update the documentation with $ make html from within the hnn-core/doc/ directory. Any idea what is going on?

# Python version: 3.7.7 (CPython)
# Docutils version: 0.16 release
# Jinja2 version: 2.11.1
# Last messages:

# Loaded extensions:
Traceback (most recent call last):
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx/cmd/build.py", line 275, in build_main
    args.tags, args.verbosity, args.jobs, args.keep_going)
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx/application.py", line 278, in __init__
    self._init_builder()
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx/application.py", line 334, in _init_builder
    self.events.emit('builder-inited')
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx/events.py", line 99, in emit
    results.append(callback(self.app, *args))
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx_gallery/gen_gallery.py", line 291, in generate_gallery_rst
    gallery_conf = parse_config(app)
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx_gallery/gen_gallery.py", line 91, in parse_config
    abort_on_example_error, lang, app.builder.name, app)
  File "/home/ryan/anaconda3/envs/hnn_core/lib/python3.7/site-packages/sphinx_gallery/gen_gallery.py", line 215, in _complete_gallery_conf
    "found type %s" % type(backref))
ValueError: The 'backreferences_dir' parameter must be of type str, pathlib.Path or None, found type <class 'bool'>

jasmainak · 2020-04-10T03:44:26Z

I think we need to update that in conf.py. There have been some changes in sphinx-gallery ...

jasmainak · 2020-05-05T00:15:39Z

You need to rebase @rythorpe ? Are you able to do it yourself?

Also -- I think we need to discuss this PR in person. Maybe on Thursday if you have time?

rythorpe · 2020-05-05T01:40:08Z

You need to rebase @rythorpe ? Are you able to do it yourself?

Also -- I think we need to discuss this PR in person. Maybe on Thursday if you have time?

I can rebase, though I don't understand why? Sure, Thursday works for me.

jasmainak · 2020-05-05T02:14:32Z

You need to rebase because there are conflicts now. You edited the same file in both the pull requests.

jasmainak · 2020-05-05T02:14:59Z

Please make a back up of your branch before you begin rebasing. It might be a mess if you are trying the first time

rythorpe · 2020-05-05T16:55:11Z

@jasmainak Any idea what is going wrong with the circleci error?

jasmainak · 2020-05-05T16:55:59Z

Ignore it for now, I have to configure CircleCI ...

jasmainak · 2020-05-18T18:52:33Z

hnn_core/__init__.py

@@ -2,7 +2,7 @@

 load_custom_mechanisms()

-from .dipole import simulate_dipole
+from .dipole import simulate_dipole, import_dipole


Suggested change

from .dipole import simulate_dipole, import_dipole

from .dipole import simulate_dipole, read_dipole

also update api.rst

jasmainak · 2020-05-18T18:56:39Z

hnn_core/network.py

+            (i.e., as a trial) to the spike-related
+            attributes of Network instance. If False,
+            all spikes of the Network instance will
+            be overwritten.


Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>

rythorpe · 2020-06-12T14:07:39Z

@jasmainak I've added gid_dict validation for Spikes().update_types() so that it checks for overlapping gid ranges. Note that I also shifted read_spikes() to create a Spikes object and then run Spikes().update_types() when supplied with the gid_dict arg (i.e., as opposed to having read_spikes() run its own code to populate the Spikes()._types attribute) to consolidate code. api.rst and whats_new.rst are updated as well.

hnn_core/network.py

jasmainak · 2020-06-12T14:47:51Z

hnn_core/network.py

+        times_self = [[round(time, 3) for time in trial]
+                      for trial in self._times]
+        times_other = [[round(time, 3) for time in trial]
+                       for trial in other._times]
+        return (times_self == times_other and
+                self._gids == other._gids and
+                self._types == other._types)


I think if you did np.allclose(times_self, times_other, atol=1e-3, rtol=0) and self._types == other._types it would do the same thing?

I tried it in the beginning but didn't care for the fact that np.allclose() uses broadcasting (i.e., it won't control for the case when there has been a formatting discrepancy between self._times and other._times.

humm ... but you won't have any formatting discrepancy now? Because you do input validation.

There shouldn't be any formatting discrepancies due to inputs, but if a bug were to emerge in Network that results in a formatting discrepancy (e.g., if someone were to mess with some square brackets so that the lists append along the first dimension), it would benefit us to have an __eq__() function that is independent. What if we use np.array_equal()?

ufff right, the Network class. Okay let's leave it like this for now. We can revisit it later.

hnn_core/network.py

jasmainak · 2020-06-12T14:54:04Z

@rythorpe just two comments. Many tests added, we're going in the right direction!

LGTM once the two comments are addressed

hnn_core/network.py

jasmainak · 2020-06-12T15:04:38Z

That's it from me, let's try to merge this PR as soon as possible so we can move on :)

cjayb

Just two minor comments.

cjayb · 2020-06-12T15:28:01Z

examples/plot_simulate_evoked.py

+with tempfile.TemporaryDirectory() as tmp_dir_name:
+    print(tmp_dir_name)
+    net.spikes.write(op.join(tmp_dir_name, 'spk_%d.txt'))
+    spikes = read_spikes(op.join(tmp_dir_name, 'spk_*.txt'))
+spikes.plot()


I get that it's good to demo the use of the write method and read_spikes function. But I wonder if it confuses the new user (likely to look at this example first). The writing to and reading from disk are not needed for plotting. Maybe both? First net.spikes.plot(), then the I/O-bit?

cjayb · 2020-06-12T15:34:47Z

hnn_core/tests/test_compare_hnn.py

@@ -36,7 +36,7 @@ def test_hnn_core():

    # Test spike type counts
    spiketype_counts = {}
-    for spikegid in net.spikegids[0]:
+    for spikegid in net.spikes.gids[0]:


I'm sorry if this is obvious, but what's the difference between spikes.gids and spikes._gids?

No difference. It's a safety mechanism so that the user doesn't modify spikes.gids.

cjayb · 2020-06-12T15:42:32Z

L really GTM, my coding skills don't warrant deeper nit-picks.

jasmainak · 2020-06-12T15:44:26Z

Thanks for taking a look!

@rythorpe the CIs are failing

rythorpe · 2020-06-12T15:49:27Z

@jasmainak it turns out that there was a reason for the nested statement

rythorpe · 2020-06-12T15:56:45Z

@jasmainak last thing, I just added the inputs and outputs section to api.rst as you had recommended earlier

rythorpe · 2020-06-12T16:27:10Z

@jasmainak I think we're good to go. Do you want to move forward with the merge?

jasmainak · 2020-06-12T16:31:32Z

Couple of things for future:

Inputs and Outputs should probably be formatted as a subheadheading: https://54-168215891-gh.circle-artifacts.com/0/html/api.html. Also read_params should be under it. We could have another subheading for cells. Anyway, future work :) I'm really done with this PR. Let's merge

jasmainak · 2020-06-12T16:31:53Z

Merged, thanks @rythorpe for the contribution!

jasmainak reviewed Mar 27, 2020

View reviewed changes

jasmainak reviewed Mar 28, 2020

View reviewed changes

hnn_core/dipole.py Show resolved Hide resolved

jasmainak reviewed Mar 30, 2020

View reviewed changes

jasmainak force-pushed the write_methods branch from 60ce1b2 to a1d4219 Compare April 5, 2020 19:41

rythorpe force-pushed the write_methods branch 2 times, most recently from a0a8b1b to 255f79c Compare April 10, 2020 03:02

rythorpe force-pushed the write_methods branch from b97e104 to f5b5c5c Compare May 5, 2020 16:39

jasmainak reviewed May 18, 2020

View reviewed changes

rythorpe force-pushed the write_methods branch from f5b5c5c to 89d0c98 Compare June 1, 2020 18:00

rythorpe and others added 5 commits June 11, 2020 20:53

Apply suggestions from code review

3d8945f

Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>

apply more suggestions from code review

42e29a6

write to temporary directory in plot_simulate_evoked.py

437c2aa

update api.rst and whats_new.rst

aacf242

add test for Spikes().__repr__()

6798516

rythorpe force-pushed the write_methods branch from f5fde6f to 6798516 Compare June 12, 2020 11:52

validate gid_dict in Spikes().update_types()

87bf7c7

jasmainak reviewed Jun 12, 2020

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

jasmainak reviewed Jun 12, 2020

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

jasmainak reviewed Jun 12, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak reviewed Jun 12, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

cjayb approved these changes Jun 12, 2020

View reviewed changes

make changes from review

daef135

rythorpe force-pushed the write_methods branch from 483ffb5 to daef135 Compare June 12, 2020 15:48

add inputs and outputs section to api.rst

4bbe726

plot spikes before I/O in evoked example

d45d8aa

jasmainak merged commit 5aa5610 into jonescompneurolab:master Jun 12, 2020

rythorpe deleted the write_methods branch June 12, 2020 16:34

rythorpe mentioned this pull request Jun 17, 2020

write txt files containing dipoles and spiking #95

Closed

	from .dipole import simulate_dipole, import_dipole
	from .dipole import simulate_dipole, read_dipole

Fix dipole and network (spiking) class write methods #96

Fix dipole and network (spiking) class write methods #96

Conversation

rythorpe commented Mar 27, 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

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

blakecaldwell commented Mar 27, 2020

jasmainak commented Mar 28, 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 Apr 2, 2020

jasmainak commented Apr 5, 2020

rythorpe commented Apr 5, 2020

jasmainak commented Apr 5, 2020

rythorpe commented Apr 10, 2020

jasmainak commented Apr 10, 2020

jasmainak commented May 5, 2020

rythorpe commented May 5, 2020

jasmainak commented May 5, 2020

jasmainak commented May 5, 2020

rythorpe commented May 5, 2020

jasmainak commented May 5, 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 12, 2020

jasmainak Jun 12, 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

Choose a reason for hiding this comment

jasmainak commented Jun 12, 2020

jasmainak commented Jun 12, 2020

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

cjayb commented Jun 12, 2020

jasmainak commented Jun 12, 2020

rythorpe commented Jun 12, 2020

rythorpe commented Jun 12, 2020

rythorpe commented Jun 12, 2020

jasmainak commented Jun 12, 2020

jasmainak commented Jun 12, 2020

jasmainak Jun 12, 2020 •

edited