[MRG] fix logic checking extinput synweights zero #102

cjayb · 2020-05-25T14:02:11Z

This PR fixes the part of #98 (specifically 94fc21a in feed.py) that botches the alpha-example.

No test written yet, input appreciated! There's a refactor brewing (see also discussion in #98), so perhaps tests should be deferred? To speed up tests, would it make sense to create a 'minimal network'? Not sure that makes sense, but almost 3 min spent on just the full alpha-example isn't sustainable.

Fixes #101

jasmainak · 2020-05-25T18:58:20Z

@rythorpe do you want to review this PR since the commit that introduces the problem was yours?

jasmainak · 2020-05-25T20:03:07Z

hnn_core/feed.py

+                if self.p_ext[key][0] > 0.0:
+                    all_syn_weights_zero = False
+        if all_syn_weights_zero:
+            return False


it's a bit weird that this method even returns something because the return value is not used anywhere

@cjayb I see, so the error in #101 is caused by one of the synaptic weights from 'OnlyRhythmicDist.param' being zero, thus causing __create_extinput() to return False even though some of the other weights are non-zero?

To address @jasmainak's point, this might be a good time to rethink the return values of the __create_ methods in the ExtFeed class. It looks like the code is currently set up to return the number of inputs added to the feed (for troubleshooting purposes?), though this might be unnecessary if we implement good tests.

yes, +10 for tests! @rythorpe do you have a suggestion for a test based on your experience in #99 ?

so the error in #101 is caused by one of the synaptic weights from 'OnlyRhythmicDist.param' being zero

Correct, these are the only non-zero weights:

'input_dist_A_weight_L2Pyr_ampa': 5.4e-5, 'input_dist_A_weight_L5Pyr_ampa': 5.4e-5,

The method would return unless all weights were non-zero.

rethink the return values of the __create_ methods in the ExtFeed class

+1 It doesn't seem that the return values are ever used

Follow the loop logic: it should only end the function if all inputs are zero. Before it would end if any input was <=0.
I’ll update the rst, but I think this was an unintentional mod (bug)

okayyyy, now I get it

so all you need to do for the unit test is to initialize an ExtFeed object with the right parameters and then check if self.eventvec exists (or is not None. I didn't check exactly whether this class initializes all attributes to None or adds them as and when needed). No computation required at all.

I’ll update the rst, but I think this was an unintentional mod (bug)

yep it should go to the bug section of whats_new.rst

Continuing off @jasmainak's suggestion, ideally we'd check the number and type of inputs in the p_ext attribute of an ExtFeed object so that we can be sure all (vs. any) expected inputs were added to the feed. I think this test could be added to the network test script.

see my latest push: it wasn't enough to check p_ext, since these could be copied in fine, despite __create_extinput not running through to create the eventvec. I think we should test p_ext in create_pext?

cjayb · 2020-05-26T18:11:18Z

Re: tests, I just did

    params_fname = op.join(hnn_core_root, 'param', 'default.json')
    params = read_params(params_fname)
    params.update({'input_dist_A_weight_L2Pyr_ampa': 5.4e-5,
                                 'input_dist_A_weight_L5Pyr_ampa': 5.4e-5,
                                 't0_input_dist': 50})
...

which adds 'extinput': 33, to spiketype_counts. The assertions against dpl_master will fail, but I'm pretty certain this would have caught the bug in #101.

Still don't like the length of this test, though... Just for the heck of it, I tried

params.update({"N_pyr_x": 2, "N_pyr_y": 2})

and re-run simulate_dipole. The model compiled and ran in just a few seconds, and produced some output

spiketype_counts
{'extinput': 34,
 'evprox1': 11,
 'L5_basket': 5,
 'L2_pyramidal': 13,
 'L2_basket': 6,
 'L5_pyramidal': 31,
 'evdist1': 9,
 'evprox2': 11}

Could this be a strategy? Shall we close this PR, then I try to create a fast test a la the above in a separate one?

jasmainak · 2020-05-26T18:23:19Z

@cjayb I like the idea of having tests on a small network! However, I'm not sure if the signals you get out of the smaller network will be as meaningful to know if we broke something?

Re the old tests of comparing to dpl.txt, I would not touch it yet. I think once we have CircleCI running in #97 we can automatically check the plots like in MNE-Python for all the examples. But we need more tests which look like "unit" tests. That is, they check small parts of the code.

cjayb · 2020-05-26T19:40:06Z

My thought was that all the simulations are deterministic, so even though they would be physiologically meaningless, they could serve as unit-like tests? Given that most of what we do involves nrn, we’re going to have to deal with the long compile/run times somehow...

jasmainak · 2020-05-27T14:18:44Z

My thought was that all the simulations are deterministic, so even though they would be physiologically meaningless, they could serve as unit-like tests?

But that's not a very good unit test because if you fix a bug that changes the overall output, your test will fail. This is the case with the test where we compare with the old HNN output. The existing test is a bad unit test. It's just better than nothing.

cjayb · 2020-05-27T16:27:42Z

But that's not a very good unit test because if you fix a bug that changes the overall output, your test will fail.

OK, got it.

hnn_core/tests/test_network.py

jasmainak · 2020-05-27T16:48:14Z

hnn_core/tests/test_network.py

+    # Assert number of rhythmic inputs is 2 (distal & proximal)
+    assert len(net.extinput_list) == 2
+    for ei in net.extinput_list:
+        # naming could be better


yes this should be called 'rhythmic'. We've discussed this with @rythorpe !

jasmainak · 2020-05-28T05:30:12Z

@cjayb I think your PR looks good to go for me. I played around with it a bit and removed some comments. I hope that's okay for you? I prefer to see clean code through good choice of variable names than explaining it through comments :)

If you're fine, please set the PR title to [MRG] and I'll merge in the morning when I wake up. Thanks a lot for the PR. Looking forward to more contributions in the future! Maybe an easy one is to change the naming of 'extinput' -> 'rhythmic'?

cjayb · 2020-05-28T10:57:12Z

Nice touch with the context manager! I definitely don't mind the edits. Good way to work this: add copious comments while working on PR, remove most once test is written.

Sure, I'll take a stab at the renaming next. Goal will be to create some test(s) while I'm at it.

jasmainak · 2020-05-28T15:49:26Z

Perfect plan! Thanks a lot :-)

fix logic checking extinput synweights zero

a3ad094

jasmainak requested a review from rythorpe May 25, 2020 20:00

jasmainak reviewed May 25, 2020

View reviewed changes

add test on rhythmic ext inputs actually present

11b8cc2

jasmainak reviewed May 27, 2020

View reviewed changes

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

jasmainak reviewed May 27, 2020

View reviewed changes

cjayb and others added 3 commits May 27, 2020 19:33

move to new feeds test and add to whats new

106dac8

fix pytest fail due to ParallelContext open

7d043fd

STY: cosmetic improvements

fda1d35

cjayb changed the title ~~fix logic checking extinput synweights zero~~ [MRG] fix logic checking extinput synweights zero May 28, 2020

jasmainak merged commit 917aff4 into jonescompneurolab:master May 28, 2020

cjayb mentioned this pull request May 28, 2020

[MRG] cleanup variable names and logic related to "common" inputs #103

Merged

cjayb deleted the fix_alphademo_101 branch June 4, 2020 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] fix logic checking extinput synweights zero #102

[MRG] fix logic checking extinput synweights zero #102

cjayb commented May 25, 2020

jasmainak commented May 25, 2020

jasmainak May 25, 2020

rythorpe May 26, 2020

rythorpe May 26, 2020

jasmainak May 26, 2020 •

edited

cjayb May 26, 2020

cjayb May 26, 2020

jasmainak May 27, 2020 •

edited

jasmainak May 27, 2020

rythorpe May 27, 2020

cjayb May 27, 2020

cjayb commented May 26, 2020

jasmainak commented May 26, 2020

cjayb commented May 26, 2020

jasmainak commented May 27, 2020 •

edited

cjayb commented May 27, 2020

jasmainak May 27, 2020

jasmainak commented May 28, 2020

cjayb commented May 28, 2020

jasmainak commented May 28, 2020

[MRG] fix logic checking extinput synweights zero #102

[MRG] fix logic checking extinput synweights zero #102

Conversation

cjayb commented May 25, 2020

jasmainak commented May 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak May 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak May 27, 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

cjayb commented May 26, 2020

jasmainak commented May 26, 2020

cjayb commented May 26, 2020

jasmainak commented May 27, 2020 • edited

cjayb commented May 27, 2020

Choose a reason for hiding this comment

jasmainak commented May 28, 2020

cjayb commented May 28, 2020

jasmainak commented May 28, 2020

jasmainak May 26, 2020 •

edited

jasmainak May 27, 2020 •

edited

jasmainak commented May 27, 2020 •

edited