REVIEW: mb/usability #53

mbeyeler · 2016-11-24T03:41:33Z

A number of features that are meant to improve the usability of pulse2percept:

There should be a Jupyter notebook that showcases the current, up-to-date usage of the model: examples/notebook/0.0-example-usage.ipynb. This will be useful as we continue to change the functionality and syntax of the model.
joblib and dask are now optional.

It's now super-easy to create common electrode arrays, such as an Argus I with a given center location (x_center, y_center), a height h (either a list or a scalar), and a rotation angle rot:

argus = e2cm.ArgusI(x_center, y_center, h, rot)

Instead of stuff like this:

# Location on retina for each electrode
x_elec_loc = np.array([-6825.092215, -6332.563035, -5840.033855, -5347.504675,
                    -6194.683612, -5702.154432, -5209.625252, -4717.096072,
                    -5564.275010, -5071.745829, -4579.216649, -4086.687469,
                    -4933.866407, -4441.337226, -3948.808046, -3456.278866])
y_elec_loc = np.array([-655.666769, -25.258166, 605.150437, 1235.559040,
                    -1148.195949, -517.787346, 112.621257, 743.029860,
                    -1640.725129, -1010.316526, -379.907924, 250.500679,
                    -2133.254310, -1502.845707, -872.437104, -242.028501])

# Alternate electrode size (Argus I)
r_arr = np.array([260, 520, 260, 520]) / 2.0
r_arr = np.concatenate((r_arr, r_arr[::-1], r_arr, r_arr[::-1]), axis=0)
h_arr = np.ones(16)*100
argus = e2cm.ElectrodeArray(r_arr.tolist(), x_elec_loc, y_elec_loc, h_arr.tolist())

ArgusI is an instance of type ElectrodeArray. It can automatically figure out the size and retinal location of each electrode in the array. No more math by hand.

The same is true for e2cm.ArgusII.

ElectrodeArray is now indexable. Electrodes can be addressed either by index or by name:

# An ArgusI array centered on the fovea
argus = e2cm.ArgusI()

# Get the second electrode in the array, by index or by name:
el = argus[1]
el = argus['A2']

# Find the index of an electrode:
el_idx = argus.get_index('C3')

ElectrodeArray no longer requires all arguments to be lists. Now supports ints, floats, lists, and numpy arrays as inputs. Electrode type is mandatory and comes first (to avoid confusion between epiretinal and subretinal arrays). For example, all the following are now possible:
```
implant = e2cm.ElectrodeArray('epiretinal', 0, 1, 2, 3)
implant = e2cm.ElectrodeArray('subretinal', [0, 0], [1, 1], [2, 2], [3, 3])
# If you must:
implant = e2cm.ElectrodeArray('epiretinal', 0, [1], np.array([2]), np.array([[3]]))
```
The ec2b.pulse2percept call now accepts an input stimulus stim, an electrode array implant, a temporal model tm, and a retina retina. If not specified, a temporal model with default parameters is used. If not specified, a retina large enough to fit the electrode array is used. dolayer is no longer specified, since it is redundant (depends on the electrode type of implant). Order of arguments has changed, too:
```
def pulse2percept(stim, implant, tm=None, retina=None,
                  rsample=30, scale_charge=42.1, tol=0.05, use_ecs=True,
                  engine='joblib', dojit=True, n_jobs=-1):
```
Input arguments are type checked in order to make it easier to spot examples with outdated API calls.

Input stimuli specification is much more flexible. Single-electrode arrays take just a single pulse train. For multi-electrode arrays, the user can specify a list of pulse trains, or specify all electrodes that should receive non-zero pulse trains by name:

# Create an Argus I array centered on the fovea
implant = e2cm.ArgusI()

# Send some non-zero pulse train to electrodes 'C2' and 'D3'. All other
# electrodes get zeros
pt = e2cm.Psycho2Pulsetrain(tsample=5e-6, freq=50, amp=20)
stim = {'C2': pt, 'D3': pt}
ec2b.pulse2percept(stim, implant)

The function ec2b.get_brightest_frame finds and returns the frame in a brightness movie that contains the brightest pixel.
The retina object now automatically generates a descriptive filename based on the input arguments (e.g., '../retina_s250_l2.0_rot0.0_5000x5000.npz'), and checks if the file exists. The retina can now easily be rotated by specifying some angle rot. Axon maps, rot, and axon_lambda are now all members of the object.
You can now have single-pixel retinas, which is handy for model-tuning when you only need the brightest pixel.
All new functions have their own test cases, with increased coverage.

…map user-accessible

coveralls · 2016-11-25T21:31:46Z

Coverage increased (+4.6%) to 85.411% when pulling 8e03145 on mbeyeler:mb/usability into 3063434 on uwescience:master.

arokem

I realize this is still WIP. For now, just a couple of small comments.

I think that we might want to think about a future uniform API where one object or kind of object (maybe the ArgusI object and similar) serves as an interface and does all the work through a uniform set of functions. For example, have every one of our ElectrodeArray sub-types have a predict method (or some-such) that generates the percepts. Just a thought.

arokem · 2016-11-24T19:43:37Z

pulse2percept/electrode2currentmap.py

        """
        Initialize an electrode object

        Parameters
        ----------
-        radius : float
+        r : float


Would be good to add documentation for the ptype parameter

arokem · 2016-11-24T19:45:11Z

pulse2percept/electrode2currentmap.py

@@ -182,24 +187,81 @@ class ElectrodeArray(object):
    """
    Represent a retina and array of electrodes
    """
+    import operator


Doesn't look like this parameter gets used

Good catch!

arokem · 2016-11-24T19:47:05Z

pulse2percept/electrode2currentmap.py

+                               axis=0)
+
+        # For now, all electrodes have the same height
+        h_arr = np.ones_like(r_arr) * h


Maybe we can test to see whether this is an array or a scalar and then do this only if this is a scalar (asserting that the shape is correct otherwise)?

arokem · 2016-11-26T00:29:38Z

.travis.yml

@@ -2,7 +2,7 @@ sudo: false

 env:
  global:
-    - CONDA_DEPS="pip flake8 numpy scipy numba joblib dask" PIP_DEPS="pytest coveralls pytest-cov"
+    - CONDA_DEPS="pip flake8 numpy scipy numba joblib dask nose" PIP_DEPS="pytest coveralls pytest-cov"


Why is nose required? We are using pytest for testing.

(the reason for that is that nose is probably going away some time in the future: http://nose.readthedocs.io/en/latest/#note-to-users)

Travis told me. If you look at the output of testing 3ab0fab, it says at the bottom: ImportError: Need nose >= 1.0.0 for tests - see http://somethingaboutorange.com/mrl/projects/nose. It looks like it has to do with npt.assert_raises. Once I included nose, the error went away.

Gotcha - makes sense. We might need to eventually shift to using the pytest error assertions. Something like this, I believe: http://stackoverflow.com/questions/23337471/how-to-properly-assert-that-exception-raises-in-pytest

coveralls · 2016-12-02T06:21:15Z

Coverage increased (+4.3%) to 85.099% when pulling e771811 on mbeyeler:mb/usability into 3063434 on uwescience:master.

… calls

coveralls · 2016-12-07T04:36:16Z

Coverage increased (+6.1%) to 86.937% when pulling 4a99791 on mbeyeler:mb/usability into 3063434 on uwescience:master.

coveralls · 2016-12-07T05:55:02Z

Coverage increased (+6.4%) to 87.245% when pulling 1c344be on mbeyeler:mb/usability into 3063434 on uwescience:master.

coveralls · 2016-12-07T22:50:47Z

Coverage increased (+5.8%) to 86.557% when pulling bf7e772 on mbeyeler:mb/usability into 3063434 on uwescience:master.

Next, to fix the broken docstring tests...

I think that the remaining failure is a real bug.

arokem

This is great. The new API looks pleasant to use and the addition of tests/examples is excellent. This is definitely going in the right direction. I had a few small comments.

We also still need to deal with the small issue that I identified in mbeyeler#1

(btw - you might want to turn on Travis on your own fork, so that it tests my PRs on top of your branches)

(p.p.s What does the mb/branch_name nomenclature mean, and how do you use it?)

arokem · 2016-12-07T23:57:46Z

pulse2percept/effectivecurrent2brightness.py

-                 lweight=0.636, epsilon=8.617,
-                 asymptote=1.0, slope=3.0, shift=15.0,
-                 scale_slow=521.0):
+                 lweight=0.636, scale_slow=1150.0,


The docstring is now out of sync with the parameter names. Docstring has tau1, and doesn't have tau_nfl and tau_inl

arokem · 2016-12-07T23:58:40Z

pulse2percept/effectivecurrent2brightness.py

+    scale_charge : float
+        Scaling factor applied to charge accumulation (used to be called
+        epsilon). Default: 42.1.
+    tol : float


Short explanation on each one of these?

arokem · 2016-12-08T00:00:33Z

pulse2percept/effectivecurrent2brightness.py

-    for p in range(len(ptrain)):
-        ca = tm.tsample * np.cumsum(np.maximum(0, -ptrain[p].data))
+    # Apply charge accumulation
+    for i, p in enumerate(pt_list):


More pythonic every day!

Looks much better, doesn't it. :)

arokem · 2016-12-08T00:02:23Z

pulse2percept/effectivecurrent2brightness.py

+        There are several ways to specify an input stimulus:
+        * For a single-electrode array, pass a single pulse train; i.e., a
+          single utils.TimeSeries object.
+        * For a multi-electrode array, pass a list of pulse trains; i.e., one


Where every pulse train is a TimeSeries object?

arokem · 2016-12-08T00:05:56Z

pulse2percept/oyster.py

-                center=np.array([15, 2]),
-                rot=0 * np.pi / 180, scale=1, bs=-1.9, bi=.5, r0=4,
-                maxR=45, angRange=60):
+def makeAxonMap(xg, yg, jan_x, jan_y, axon_lambda=1, min_weight=.001):


The combination of setting nCells and nR as key-word arguments to jansonius and removing them as inputs here is a bit like hard-coding these values. Because the API doesn't let you change these values. Maybe not a big deal, if there is a compelling argument for this change.

The goal here was to give a user access to the axon map (jan_x and jan_y) for visual investigation and pretty-plotting, which is the output of jansonius. In the old code, the Retina object called makeAxonMap, which called jansionius under the hood, therefore not exposing jan_x and jan_y to theuser. Now Retina calls both these functions, and stores jan_x and jan_y in the .npz file.

Funny enough, nCells and nR were never available to the Retina API, so I hardcoded them. I don't think they are supposed to change, but I can always bring them back if we need them.

arokem · 2016-12-08T00:06:51Z

pulse2percept/oyster.py

@@ -137,10 +134,6 @@ def makeAxonMap(xg, yg, axon_lambda=1, nCells=500, nR=801, min_weight=.001,
        space

    """
-    axon_x, axon_y = jansonius(nCells, nR, center=center,


But I might be missing something (re: key-word arguments), given that you removed the call to jansonius. What happened here?

See my comment above

arokem · 2016-12-08T00:07:48Z

pulse2percept/tests/test_effectivecurrent2brightness.py

    sampling = 1
    xlo = -2
    xhi = 2
    ylo = -3
    yhi = 3
    retina = e2cm.Retina(xlo=xlo, xhi=xhi, ylo=ylo, yhi=yhi,
-                         sampling=sampling, axon_map=retina_file)
+                         sampling=sampling, loadpath='')


This will create a file wherever users are running their tests, wouldn't it? Better to create a temp-file to avoid strange things happening when this is run in a location where a file already exists?

I could try to argue about how unlikely that is with the name filename generator, or start messing with tempdirs... :) But a much easier solution seemed to be the introduction of an optional input argument save_data that is set to True by default but gets set to False in all tests. Hence no new files will be generated in tests.

A few small testing fixes...

coveralls · 2016-12-08T05:37:04Z

Coverage increased (+5.8%) to 86.641% when pulling 03d84f0 on mbeyeler:mb/usability into 3063434 on uwescience:master.

coveralls · 2016-12-08T06:30:04Z

Coverage increased (+3.7%) to 84.461% when pulling 38f48a1 on mbeyeler:mb/usability into 3063434 on uwescience:master.

arokem · 2016-12-08T15:56:11Z

Looks good to me! Thanks for the additional explanation re: jansonius/makeAxonMap.

I like the save_data solution to caching of retina information.

This should be ready to go. Since I am going to see you in a couple of hours, I'll wait to touch base with you before hitting the merge button.

arokem · 2016-12-08T16:01:23Z

Oh dear. I am still getting the following failure when running the tests locally (including doctests):


D-69-91-145-134:pulse2percept (mb/usability) $py.test --pyargs pulse2percept --cov-report term-missing --cov=pulse2percept --doctest-modules
============================= test session starts ==============================
platform darwin -- Python 3.5.1, pytest-2.8.3, py-1.4.30, pluggy-0.3.1
rootdir: /Users/arokem/source/pulse2percept, inifile: 
plugins: cov-2.2.0
collected 21 items 

pulse2percept/effectivecurrent2brightness.py F
pulse2percept/electrode2currentmap.py ...
pulse2percept/tests/test_effectivecurrent2brightness.py ....
pulse2percept/tests/test_electrode2currentmap.py ..........
pulse2percept/tests/test_utils.py ...

=================================== FAILURES ===================================
______ [doctest] pulse2percept.effectivecurrent2brightness.pulse2percept _______
304     -------
305     A brightness movie depicting the predicted percept, running at `rsample`
306     frames per second.
307 
308     Examples
309     --------
310     Stimulate a single-electrode array:
311     >>> implant = e2cm.ElectrodeArray('subretinal', 0, 0, 0, 0)
312     >>> stim = e2cm.Psycho2Pulsetrain(tsample=5e-6, freq=50, amp=20)
313     >>> pulse2percept(stim, implant)
Expected nothing
Got:
    File '../retina_s25_l2.0_rot0.0_1000x1000.npz' doesn't exist or has outdated parameter values, generating...
    <pulse2percept.utils.TimeSeries object at 0x10cd82a90>

/Users/arokem/source/pulse2percept/pulse2percept/effectivecurrent2brightness.py:313: DocTestFailure
--------------- coverage: platform darwin, python 3.5.1-final-0 ----------------
Name                                                      Stmts   Miss  Cover   Missing
---------------------------------------------------------------------------------------
pulse2percept/__init__.py                                     6      0   100%   
pulse2percept/effectivecurrent2brightness.py                133      9    93%   349, 365, 391-392, 460-461, 484-492
pulse2percept/electrode2currentmap.py                       299     43    86%   32-33, 149-151, 189-191, 378-381, 467-470, 507-514, 521-523, 530-535, 569, 660-661, 670, 679, 684, 697, 703-704, 787, 803, 818, 946-952
pulse2percept/files.py                                       23     16    30%   21-23, 27-45, 55-60
pulse2percept/oyster.py                                      67      0   100%   
pulse2percept/tests/__init__.py                               0      0   100%   
pulse2percept/tests/test_effectivecurrent2brightness.py      98      1    99%   116
pulse2percept/tests/test_electrode2currentmap.py            229      0   100%   
pulse2percept/tests/test_utils.py                            33      0   100%   
pulse2percept/utils.py                                      129     55    57%   10-11, 29, 83, 122-124, 196-199, 208-226, 241-257, 267-279
pulse2percept/version.py                                     32      1    97%   14
---------------------------------------------------------------------------------------
TOTAL                                                      1049    125    88%   
===================== 1 failed, 20 passed in 79.94 seconds =====================

arokem · 2016-12-08T16:01:44Z

On the bright side - 88% coverage!

mbeyeler · 2016-12-08T17:32:24Z

Although now all tests pass locally, somehow doctest: +ELLIPSIS doesn't do anything to ignore that one output line when Travis runs the tests... Any idea?

coveralls · 2016-12-08T19:54:13Z

Coverage increased (+5.2%) to 85.987% when pulling 7b4f70a on mbeyeler:mb/usability into 3063434 on uwescience:master.

Michael Beyeler added 9 commits November 23, 2016 15:56

expose retina rotation to constructor call

14e23f4

make joblib and dask optional

d88613c

make ElectrodeArray accept ints, floats, lists, np.arrays

8868088

fix tests

5ba7b76

add ArgusI ElectrodeArray

582f7a7

fix tests

3ab0fab

update example ipynb; automate retina filename generation; make axon …

d47c90f

…map user-accessible

fix tests

0dbbeec

fix flake8 complaints

8e03145

arokem reviewed Nov 26, 2016

View reviewed changes

Michael Beyeler added 2 commits November 26, 2016 13:32

Implement review comments

d4a9944

fix charge accumulation and scale_out

e771811

mbeyeler changed the title ~~Work in progress: mb/usability~~ WIP: mb/usability Dec 5, 2016

add indexable ElectrodeArray; add deBalthasar test; refactor some API…

4a99791

… calls

Michael Beyeler added 2 commits December 6, 2016 21:24

add simplified pulse2percept call

1fa81e2

fix Travis complaints

1c344be

Michael Beyeler added 3 commits December 7, 2016 13:33

fix Argus I indexing; typecheck pulse2percept and add tests

6375593

add Argus II array, example, and test case

e1cd6c3

remove trailing whitespace

bf7e772

mbeyeler changed the title ~~WIP: mb/usability~~ REVIEW: mb/usability Dec 7, 2016

mbeyeler requested a review from arokem December 7, 2016 22:48

arokem added 2 commits December 7, 2016 16:45

TST: Remove nose dependency. Implement docstring testing.

e71b29c

Next, to fix the broken docstring tests...

TST: Fix up doctests a bit.

92e472e

I think that the remaining failure is a real bug.

arokem requested changes Dec 8, 2016

View reviewed changes

Merge pull request #1 from arokem/ar/mb/usability

b2e8888

A few small testing fixes...

make ElectrodeArray iterable; fix pytest import path

03d84f0

implement reviewer comments

38f48a1

arokem approved these changes Dec 8, 2016

View reviewed changes

Michael Beyeler added 3 commits December 8, 2016 09:11

fix doctest failure

2278b06

add doctest ellipsis

c1f5ef5

please, dear doctest, play nice now.

3226ea4

Michael Beyeler added 2 commits December 8, 2016 09:49

try to fix ellipsis

884e03e

add verbose flag to suppress output during doctests

7b4f70a

arokem merged commit 9315dec into pulse2percept:master Dec 8, 2016

mbeyeler deleted the mb/usability branch June 22, 2017 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REVIEW: mb/usability #53

REVIEW: mb/usability #53

mbeyeler commented Nov 24, 2016 •

edited

coveralls commented Nov 25, 2016 •

edited

arokem left a comment

arokem Nov 24, 2016

mbeyeler Nov 26, 2016

arokem Nov 24, 2016

mbeyeler Nov 26, 2016

arokem Nov 24, 2016

arokem Nov 26, 2016

mbeyeler Nov 26, 2016

arokem Nov 27, 2016

coveralls commented Dec 2, 2016 •

edited

coveralls commented Dec 7, 2016 •

edited

coveralls commented Dec 7, 2016 •

edited

coveralls commented Dec 7, 2016 •

edited

arokem left a comment

arokem Dec 7, 2016

arokem Dec 7, 2016

arokem Dec 8, 2016

mbeyeler Dec 8, 2016

arokem Dec 8, 2016

arokem Dec 8, 2016

mbeyeler Dec 8, 2016

arokem Dec 8, 2016

mbeyeler Dec 8, 2016

arokem Dec 8, 2016

mbeyeler Dec 8, 2016

coveralls commented Dec 8, 2016 •

edited

coveralls commented Dec 8, 2016 •

edited

arokem commented Dec 8, 2016

arokem commented Dec 8, 2016

arokem commented Dec 8, 2016

mbeyeler commented Dec 8, 2016

coveralls commented Dec 8, 2016 •

edited

REVIEW: mb/usability #53

REVIEW: mb/usability #53

Conversation

mbeyeler commented Nov 24, 2016 • edited

coveralls commented Nov 25, 2016 • edited

arokem 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

coveralls commented Dec 2, 2016 • edited

coveralls commented Dec 7, 2016 • edited

coveralls commented Dec 7, 2016 • edited

coveralls commented Dec 7, 2016 • edited

arokem 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

coveralls commented Dec 8, 2016 • edited

coveralls commented Dec 8, 2016 • edited

arokem commented Dec 8, 2016

arokem commented Dec 8, 2016

arokem commented Dec 8, 2016

mbeyeler commented Dec 8, 2016

coveralls commented Dec 8, 2016 • edited

mbeyeler commented Nov 24, 2016 •

edited

coveralls commented Nov 25, 2016 •

edited

coveralls commented Dec 2, 2016 •

edited

coveralls commented Dec 7, 2016 •

edited

coveralls commented Dec 7, 2016 •

edited

coveralls commented Dec 7, 2016 •

edited

coveralls commented Dec 8, 2016 •

edited

coveralls commented Dec 8, 2016 •

edited

coveralls commented Dec 8, 2016 •

edited