postprocessing #188

kohl-carmen · 2020-09-24T17:39:01Z

Closes #183
closes #172

jasmainak · 2020-09-24T19:09:54Z

@blakecaldwell do you mind taking a look to see if it addresses what you had in mind for #183 ?

@kohl-carmen one thing that remains to be checked is what does the GUI output as rawdpl.txt, if it's baseline renormalized or not?

jasmainak · 2020-09-24T19:11:14Z

another thing to do here is to add a test for the new postproc parameter.

jasmainak · 2020-09-24T19:11:45Z

hnn_core/pyramidal.py

@@ -19,7 +19,6 @@

 class Pyr(_Cell):
    """Pyramidal neuron.
-


changes are unrelated to the PR, so best to undo them as we discussed!

blakecaldwell · 2020-10-01T14:53:06Z

Just getting around to reviewing this. Sorry for the delay. Yes, this is looking very good and should resolve #183 with some minor updates. We briefly touched on this on Monday, but I have some specific suggestions now.

Since HNN GUI needs to write both rawdpl.txt and dpl.txt, I think this could be done by making a postproc() member of Dipole that gets called in simulate when post_proc=True. This would allow HNN GUI to call simulate_dipole(..., post_proc=False), write rawdpl.txt, and then dpl.postproc(params) before writing the dpl.txt file.

def postproc(self, params):
    dpl.baseline_renormalize(params)
    dpl.convert_fAm_to_nAm()
    dpl.scale(params['dipole_scalefctr'])
    dpl.smooth(params['dipole_smooth_win'] / params['dt'])

Note, the relevant params could be passed instead of the dictionary, perhaps preferable? I didn't look up what params baseline_renormalize needs.

blakecaldwell · 2020-10-01T14:55:58Z

hnn_core/network_builder.py

-        if neuron_net.net.params['save_dpl']:
-            dpl.write('rawdpl.txt')

        dpl.baseline_renormalize(neuron_net.net.params)
        dpl.convert_fAm_to_nAm()
-        dpl.scale(neuron_net.net.params['dipole_scalefctr'])
-        dpl.smooth(neuron_net.net.params['dipole_smooth_win'] / h.dt)
+        if postproc:
+            dpl.scale(neuron_net.net.params['dipole_scalefctr'])
+            dpl.smooth(neuron_net.net.params['dipole_smooth_win'] / h.dt)


if postproc: self.postproc(params)

jasmainak · 2020-10-01T16:05:14Z

I see, I would make it _postproc so it's a private method used only in the GUI. Don't want to encourage the pattern of users calling foo_function(params) from their Python scripts. Alternatively, if it makes sense, as you suggested, we can have explicitly the parameters for all the postprocessing methods ...

@kohl-carmen I saw you pushed some new commits. Let us know when we should take a look again. I get notifications only for comments, not for commits.

blakecaldwell · 2020-10-01T16:11:47Z

I agree with using specific parameters. It somewhat negates the point below, but I think it's worth pointing out.

I blanket disagree with making functions that HNN use private. HNN should be considered a first-class use case of hnn-core.

jasmainak · 2020-10-01T16:14:50Z

Sure, of course HNN is a first class citizen of hnn-core. However, the reason I suggested using a private method is that it gives us flexibility to change the internals. If users start using hnn-core by passing params, they will complain if we try to change it in the future.

codecov-io · 2020-10-22T15:18:01Z

Codecov Report

Merging #188 (b304836) into master (bc8e4f2) will decrease coverage by 0.65%.
The diff coverage is 73.40%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   68.16%   67.50%   -0.66%     
==========================================
  Files          21       21              
  Lines        2202     2219      +17     
==========================================
- Hits         1501     1498       -3     
- Misses        701      721      +20

Impacted Files	Coverage Δ
hnn_core/network_builder.py	`93.93% <ø> (+0.17%)`	⬆️
hnn_core/parallel_backends.py	`15.31% <16.66%> (-0.81%)`	⬇️
hnn_core/dipole.py	`23.65% <28.57%> (-29.68%)`	⬇️
hnn_core/tests/conftest.py	`80.48% <100.00%> (+24.93%)`	⬆️
hnn_core/tests/test_dipole.py	`100.00% <100.00%> (ø)`
hnn_core/tests/test_parallel_backends.py	`100.00% <100.00%> (+26.31%)`	⬆️

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 bc8e4f2...b304836. Read the comment docs.

jasmainak · 2020-10-22T16:10:14Z

hnn_core/dipole.py

-        params : dict
-            The parameters
+        N_pyr_x : int
+            Nr of cells (x)


Suggested change

Nr of cells (x)

Number of Pyramidal cells in x direction

add this change in next commit?

jasmainak · 2020-10-22T16:10:41Z

@kohl-carmen can you update a test?

hnn_core/parallel_backends.py

hnn_core/tests/test_dipole.py

kohl-carmen · 2020-11-19T17:11:08Z

@klankinen and I tried to fix the postproc test. We set the std of the inputs to 0, rather than changing the tolerance on assert_allclose() because we didn't know what a reasonable tolerance would be?

The postproc test is currently still in test_dipole.py but we're happy to move it (might need some help)

rythorpe · 2020-11-19T20:09:34Z

@klankinen and I tried to fix the postproc test. We set the std of the inputs to 0, rather than changing the tolerance on assert_allclose() because we didn't know what a reasonable tolerance would be?

Hmm, setting the std of the input times to 0 won't control for error induced by smoothing vs. not smoothing a given trial. My recommendation would be leave jitter in-between trials (i.e., reset start time std values to default) and solve for a reasonable tolerance as follows:

Set the smoothing kernel to be super narrow such that it only averages a few time samples surrounding the central time sample.
Given that the smoothing kernel we use is a hamming window, the maximal difference between the original central time sample and the smoothed version cannot exceed the difference between the original central time sample and the mean of all samples contained in the smoothing window.
Use (2) to compute the maximal relative difference (i.e., tolerance) between the post-processed time series and the original time series.

This method might be overkill but it should only take a few lines of code and can serve as a sanity check for any subsequent changes made to the post-processing steps in later PRs.

The postproc test is currently still in test_dipole.py but we're happy to move it (might need some help)

Idk, @jasmainak @ntolley what do you think?

jasmainak · 2020-11-19T22:17:08Z

If you guys are working together you can both get credit for commits by co-authoring them. Just wanted to put it out there for future commits. You can also edit older commits and add authors but probably not worth the effort unless @klankinen insists ;-)

jasmainak · 2020-11-19T22:24:05Z

because we didn't know what a reasonable tolerance would be?

Something reasonably small ... Is there a reason they are not exactly equal? Usually the defaults should do but if not you can look around in the codebase for tolerance used.

jasmainak · 2020-11-19T22:24:44Z

hnn_core/dipole.py

@@ -186,21 +194,21 @@ def plot(self, ax=None, layer='agg', show=True):
        """
        return plot_dipole(dpl=self, ax=ax, layer=layer, show=show)

-    def baseline_renormalize(self, params):


hnn_core/dipole.py

jasmainak · 2020-11-19T22:26:19Z

hnn_core/parallel_backends.py

+        if postproc:
+            N_pyr_x = net.params['N_pyr_x']
+            N_pyr_y = net.params['N_pyr_y']
+            winsz = net.params['dipole_smooth_win'] / net.params['dt']
+            fctr = net.params['dipole_scalefctr']
+            for idx in range(n_trials):
+                dpls[idx].post_proc(N_pyr_x, N_pyr_y, winsz, fctr)


do you think we could avoid the duplication of code between the backends by putting this inside _gather_trial_data with postproc as an argument?

jasmainak · 2020-11-19T22:28:57Z

The postproc test is currently still in test_dipole.py but we're happy to move it (might need some help)

ideally, we should avoid duplication but let's get the other stuff working first

also you need to rebase. Do you need help? I think @ntolley is an expert ;-)

last thing is you need to update whats_new.rst. Do it last to avoid having to rebase again (the file gets updated in almost every PR).

ntolley · 2020-11-20T02:39:31Z

On first glance it looks like the rebase should be pretty easy, I think most of the conflicts can be resolved by accepting both changes (one exception is adding the new argument to simulate_dipole()). Feel free to ping me if you have any questions.

rythorpe · 2020-11-20T03:05:33Z

Something reasonably small ... Is there a reason they are not exactly equal? Usually the defaults should do but if not you can look around in the codebase for tolerance used.

This probably won't work because the error here is induced by rounding error rather than by convolution with a smoothing kernel.

jasmainak · 2020-11-20T03:30:35Z

rather than by convolution with a smoothing kernel.

not sure I follow. There are two tests: 1) checking if dpl_raw and dpl are different. and 2) checking if dpl_raw.post_proc(...) gives the same result as simulate_dipole(..., post_proc=True). It's doing the same arithmetic unless I'm missing something?

rythorpe · 2020-11-20T13:41:46Z

not sure I follow. There are two tests: 1) checking if dpl_raw and dpl are different. and 2) checking if dpl_raw.post_proc(...) gives the same result as simulate_dipole(..., post_proc=True). It's doing the same arithmetic unless I'm missing something?

Oops, somehow I missed test_dipole.py L67. For some reason I thought the goal was to simultaneously test the scope of the post-processing.

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

jasmainak · 2020-11-29T21:42:23Z

hnn_core/tests/conftest.py

+@pytest.fixture(scope='module')
+def run_hnn_core():
+    def _run_hnn_core(backend=None, n_procs=None, n_jobs=1, reduced=False,
+                      record_vsoma=False, record_isoma=False):


@kohl-carmen if you add a postproc parameter here, then you can avoid duplication of code

jasmainak

PR is good for me now! I snuck in a couple of fixes related to requires_mpi4py and moving run_hnn_core into a pytest fixture.

@ntolley @blakecaldwell @rythorpe if you guys are happy please feel free to merge and/or leave comments if something is to be fixed

blakecaldwell · 2020-11-30T09:47:58Z

hnn_core/tests/test_dipole.py

-        assert len(joblib_net.cell_response.isoma[
-                   trial][gid]['soma_gabaa']) == n_times
+
+@requires_mpi4py


Nice, I didn't even realize we had a MPIBackend test in another file besides parallel_backends.py. Thanks @jasmainak for making the testing changes.

Ah those would be my changes, thanks for fixing my ad-hoc MPI tests @jasmainak.

blakecaldwell

Just have a few suggestions, mainly in the new testing code added. So close!

hnn_core/tests/test_dipole.py

blakecaldwell · 2020-11-30T09:57:22Z

hnn_core/tests/conftest.py

+
+
+@pytest.fixture(scope='module')
+def run_hnn_core():


what about putting "fixture" in this function name? Otherwise it looks like a mysterious parameter to other testing functions.

+1 @kohl-carmen can you make the changes yourself?

blakecaldwell · 2020-11-30T09:58:47Z

hnn_core/dipole.py

-        params : dict
-            The parameters
+        N_pyr_x : int
+            Nr of cells (x)


add this change in next commit?

blakecaldwell

This looks great! Thanks @kohl-carmen

blakecaldwell · 2020-11-30T18:41:32Z

@jasmainak would you do a "squash and merge" or an automatic rebase to merge this PR? Not sure what your preference is.

jasmainak · 2020-11-30T19:10:43Z

I would do a "rebase and merge". Would give @kohl-carmen the full credit for the effort over the past month. Once we have establish set of contributors, we can move to "squash and merge" model

kohl-carmen force-pushed the postproc branch from b2704be to b6007f0 Compare September 24, 2020 18:45

jasmainak reviewed Sep 24, 2020

View reviewed changes

blakecaldwell reviewed Oct 1, 2020

View reviewed changes

jasmainak added this to the 0.1 milestone Oct 9, 2020

jasmainak reviewed Oct 22, 2020

View reviewed changes

hnn_core/parallel_backends.py Show resolved Hide resolved

rythorpe reviewed Nov 13, 2020

View reviewed changes

hnn_core/tests/test_dipole.py Outdated Show resolved Hide resolved

rythorpe reviewed Nov 13, 2020

View reviewed changes

hnn_core/tests/test_dipole.py Show resolved Hide resolved

rythorpe reviewed Nov 13, 2020

View reviewed changes

hnn_core/tests/test_dipole.py Show resolved Hide resolved

jasmainak reviewed Nov 19, 2020

View reviewed changes

hnn_core/dipole.py Show resolved Hide resolved

jasmainak reviewed Nov 19, 2020

View reviewed changes

jasmainak force-pushed the postproc branch from 9e6d37f to a441f3c Compare November 22, 2020 00:39

kohl-carmen and others added 7 commits November 29, 2020 13:33

undid plot_simulate chnages

18dcb56

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

DOC: update docstring

82d8556

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

STY: flake8

5887f9c

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

added post_proc in Dipole

cf9c7b7

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

flake8 and pytest

8f97112

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

describe function arguments

691301d

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

flake8

a7f634f

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

jasmainak force-pushed the postproc branch from 98d4965 to 7d3af00 Compare November 29, 2020 18:36

jasmainak and others added 5 commits November 29, 2020 13:40

TST more tests

b6bd7a5

enable skipping MPI

8e1e80c

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

nested import in requires_mpi4py

da7a7bc

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

ENH: postproc in _gather_trial_data

cfaf74e

rebase fix

9619ea8

Co-authored-by: Kaisu Lankinen <klankinen@mgh.harvard.edu>

jasmainak force-pushed the postproc branch from 7d3af00 to da7a7bc Compare November 29, 2020 18:40

jasmainak added 3 commits November 29, 2020 13:57

FIX requires_mpi4py -> should return for all possible outcomes

f2e82e4

TST use fixtures + MPI test in separate function

e516d34

STY Flake8

e05356a

jasmainak reviewed Nov 29, 2020

View reviewed changes

kohl-carmen added 2 commits November 29, 2020 19:52

use run_hnn_core in postproc test

b97f452

update whats_new

3ad7abd

jasmainak approved these changes Nov 30, 2020

View reviewed changes

blakecaldwell reviewed Nov 30, 2020

View reviewed changes

blakecaldwell requested changes Nov 30, 2020

View reviewed changes

update conftest function names

b304836

kohl-carmen requested a review from blakecaldwell November 30, 2020 16:05

blakecaldwell approved these changes Nov 30, 2020

View reviewed changes

blakecaldwell merged commit fa9c666 into jonescompneurolab:master Nov 30, 2020

jasmainak mentioned this pull request Nov 30, 2020

fix requires_mpi4py decorator #218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

postprocessing #188

postprocessing #188

kohl-carmen commented Sep 24, 2020 •

edited by jasmainak

jasmainak commented Sep 24, 2020

jasmainak commented Sep 24, 2020

jasmainak Sep 24, 2020

blakecaldwell commented Oct 1, 2020

blakecaldwell Oct 1, 2020

jasmainak commented Oct 1, 2020

blakecaldwell commented Oct 1, 2020 •

edited

jasmainak commented Oct 1, 2020

codecov-io commented Oct 22, 2020 •

edited

jasmainak Oct 22, 2020

blakecaldwell Nov 30, 2020

jasmainak commented Oct 22, 2020

kohl-carmen commented Nov 19, 2020

rythorpe commented Nov 19, 2020 •

edited

jasmainak commented Nov 19, 2020

jasmainak commented Nov 19, 2020

jasmainak Nov 19, 2020

jasmainak Nov 19, 2020 •

edited

jasmainak commented Nov 19, 2020 •

edited

ntolley commented Nov 20, 2020

rythorpe commented Nov 20, 2020

jasmainak commented Nov 20, 2020

rythorpe commented Nov 20, 2020

jasmainak Nov 29, 2020

jasmainak left a comment

blakecaldwell Nov 30, 2020

ntolley Nov 30, 2020

blakecaldwell left a comment

blakecaldwell Nov 30, 2020

jasmainak Nov 30, 2020

blakecaldwell Nov 30, 2020

blakecaldwell left a comment

blakecaldwell commented Nov 30, 2020

jasmainak commented Nov 30, 2020

postprocessing #188

postprocessing #188

Conversation

kohl-carmen commented Sep 24, 2020 • edited by jasmainak

jasmainak commented Sep 24, 2020

jasmainak commented Sep 24, 2020

Choose a reason for hiding this comment

blakecaldwell commented Oct 1, 2020

Choose a reason for hiding this comment

jasmainak commented Oct 1, 2020

blakecaldwell commented Oct 1, 2020 • edited

jasmainak commented Oct 1, 2020

codecov-io commented Oct 22, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Oct 22, 2020

kohl-carmen commented Nov 19, 2020

rythorpe commented Nov 19, 2020 • edited

jasmainak commented Nov 19, 2020

jasmainak commented Nov 19, 2020

Choose a reason for hiding this comment

jasmainak Nov 19, 2020 • edited

Choose a reason for hiding this comment

jasmainak commented Nov 19, 2020 • edited

ntolley commented Nov 20, 2020

rythorpe commented Nov 20, 2020

jasmainak commented Nov 20, 2020

rythorpe commented Nov 20, 2020

Choose a reason for hiding this comment

jasmainak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakecaldwell 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

blakecaldwell left a comment

Choose a reason for hiding this comment

blakecaldwell commented Nov 30, 2020

jasmainak commented Nov 30, 2020

kohl-carmen commented Sep 24, 2020 •

edited by jasmainak

blakecaldwell commented Oct 1, 2020 •

edited

codecov-io commented Oct 22, 2020 •

edited

rythorpe commented Nov 19, 2020 •

edited

jasmainak Nov 19, 2020 •

edited

jasmainak commented Nov 19, 2020 •

edited