[MRG] Add probability arguments to drives API #416

ntolley · 2021-08-20T00:50:27Z

Closes #410. Thanks to #369, this task is quite straightforward. The goal of this PR is to add a probability argument to the drives API, such that net.add_evoked_drive(..., probability=0.5) randomly removes half the drive connections, and only a subset are actually targeted by the drive. Additionally, the probability argument should also accept a dictionary which specifies probabilities for each individual connection (same was the weight and delay arguments).

After completing this, I think the final task for fully merging the drives and connection API's is enabling the manual targeting of target_gids, but let's leave that as a topic of discussion for the upcoming retreat ;).

ntolley · 2021-08-20T00:56:41Z

Also I want to mention a potential concern I had that ended up not being a problem. net.connectivity contains a separate element for each receptor type, there is the potential that a given src_gid -> target_gid pair may be missing the full suite of receptors intended for that connection. For example, the GABAa and GABAb connections from the same inhibitory src_gid targeting different neurons.

Rather conveniently, this is gracefully handled by making sure the same seed is used when calling net.add_connection for the receptors contained in the same "connection class". I'll add some tests to check this explicitly, but by manual inspection the src-target pairs agree across receptors for the same connection class.

hnn_core/network.py

hnn_core/dipole.py

hnn_core/network.py

jasmainak · 2021-08-20T14:41:35Z

add a test?

jasmainak · 2021-08-20T14:43:19Z

Also, can you add closes #410 in the description of the PR ?

codecov-commenter · 2021-08-20T18:24:05Z

Codecov Report

Merging #416 (9611b3b) into master (46e26f1) will decrease coverage by 0.69%.
The diff coverage is 100.00%.

❗ Current head 9611b3b differs from pull request most recent head ec6f3b8. Consider uploading reports for the commit ec6f3b8 to get more accurate results

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
- Coverage   88.49%   87.80%   -0.70%     
==========================================
  Files          18       18              
  Lines        3267     3287      +20     
==========================================
- Hits         2891     2886       -5     
- Misses        376      401      +25

Impacted Files	Coverage Δ
hnn_core/network_models.py	`100.00% <ø> (ø)`
hnn_core/dipole.py	`91.55% <100.00%> (+0.11%)`	⬆️
hnn_core/drives.py	`92.98% <100.00%> (+0.21%)`	⬆️
hnn_core/network.py	`92.03% <100.00%> (+0.26%)`	⬆️
hnn_core/params.py	`90.90% <100.00%> (ø)`
hnn_core/viz.py	`78.15% <0.00%> (-5.14%)`	⬇️
hnn_core/cell_response.py	`83.92% <0.00%> (-0.60%)`	⬇️

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 46e26f1...ec6f3b8. Read the comment docs.

hnn_core/network.py

hnn_core/tests/test_drives.py

jasmainak · 2021-08-23T03:21:43Z

Few minor comments otherwise LGTM

I see that there are a couple of other todos in the release checklist but this was not amongst them. Would you mind updating that checklist to reflect the latest shopping list?

ntolley · 2021-08-23T14:02:05Z

Can do! And apologies going off script w.r.t. release priorities, this came up as being very necessary for some preliminary grant analysis.

jasmainak · 2021-08-25T16:57:58Z

@ntolley looks like we have a CI failure due to MNE. Can you reproduce the import issue locally (with a Python 3.6 conda env and doing pip install mne)? If so, we should report it to MNE, otherwise we can just restart the CI

ntolley · 2021-08-25T21:36:27Z

Reproduces locally, just posted an issue: mne-tools/mne-python#9700

hnn_core/tests/test_drives.py

rythorpe

Aside from my two nitpicks above, this looks good to me!

jasmainak

merge button is yours @rythorpe

ntolley · 2021-09-17T20:01:22Z

hnn_core/network.py

+                        delay=delays, lamtha=space_constant,
+                        probability=probability,
+                        conn_seed=drive['conn_seed'] + seed_increment)
+                    # Ensure that AMPA/NMDA connections target the same gids


@rythorpe I think this addresses your problem with enforcing shared connections with seeding. The connections for the same receptor are explicitly set to be equal to one another.

I additionally added seed_increment to make sure that there aren't weird structural dependencies for the random subsets. I see a PR in the future that takes care of all the seeding as we discussed previously (seeds drawn from a single RNG).

perhaps raise an issue with crosslink here to not forget?

hnn_core/tests/test_dipole.py

hnn_core/network.py

rythorpe · 2021-09-17T21:02:23Z

hnn_core/network.py

+                    if receptor_idx > 0:
+                        self.connectivity[-1]['src_gids'] = \
+                            self.connectivity[-2]['src_gids']


Is this necessary given that conn_seed applies the same seed_increment for each receptor of a given cell type?

This is actually the root of the problem we were discussing with respect to enforcing matching connections. While technically these would share the same connections when providing the same seed, it's not very transparent, so they are explicitly set equal here.

Hmm, I think the root problem is addressed by explicitly implementing (with documentation) that random connections are assigned on a cell-by-cell basis rather than a synapse-by-synapse basis. This is perfectly implemented by setting the seed increment for each cell type in an outer loop and then establishing each receptor's connection in an inner loop. That said, maybe the explicit check is good for maintaining this established convention in the future....

jasmainak · 2021-09-18T22:54:19Z

@ntolley you need to rebase

hnn_core/network.py

jasmainak · 2021-09-18T23:04:50Z

@ntolley don't forget to update whats_new.rst !

hnn_core/network.py

Co-authored-by: Mainak Jas <[email protected]>

jasmainak · 2021-09-22T05:39:25Z

@rythorpe I'm not available until Sunday due to coding sprint in another project. Merge button is yours!

rythorpe · 2021-09-22T14:51:01Z

I'll definitely be using this feature in the near future. Thanks @ntolley!!

jasmainak reviewed Aug 20, 2021

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

ntolley force-pushed the drive_probability branch from aee036b to cc2fd1c Compare August 20, 2021 14:23

ntolley mentioned this pull request Aug 20, 2021

warning for no connections in Network object #410

Closed

jasmainak reviewed Aug 20, 2021

View reviewed changes

hnn_core/dipole.py Show resolved Hide resolved

jasmainak reviewed Aug 20, 2021

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

ntolley changed the title ~~WIP: Add probability arguments to drives API~~ [MRG] Add probability arguments to drives API Aug 20, 2021

jasmainak reviewed Aug 23, 2021

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak reviewed Aug 23, 2021

View reviewed changes

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

ntolley mentioned this pull request Aug 23, 2021

release 0.2 #365

Closed

15 tasks

rythorpe reviewed Aug 26, 2021

View reviewed changes

hnn_core/tests/test_drives.py Show resolved Hide resolved

rythorpe reviewed Aug 26, 2021

View reviewed changes

hnn_core/tests/test_drives.py Show resolved Hide resolved

rythorpe approved these changes Aug 26, 2021

View reviewed changes

jasmainak approved these changes Sep 3, 2021

View reviewed changes

ntolley force-pushed the drive_probability branch from 2487c56 to e0e1099 Compare September 3, 2021 21:03

ntolley mentioned this pull request Sep 3, 2021

refine net.connectivity and pick_connection #415

Open

3 tasks

ntolley changed the title ~~[MRG] Add probability arguments to drives API~~ WIP: Add probability arguments to drives API Sep 5, 2021

ntolley force-pushed the drive_probability branch from c090fe9 to ba0bd93 Compare September 10, 2021 16:56

ntolley commented Sep 17, 2021

View reviewed changes

ntolley changed the title ~~WIP: Add probability arguments to drives API~~ [MRG] Add probability arguments to drives API Sep 17, 2021

rythorpe reviewed Sep 17, 2021

View reviewed changes

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

rythorpe reviewed Sep 17, 2021

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

rythorpe reviewed Sep 17, 2021

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

rythorpe reviewed Sep 17, 2021

View reviewed changes

jasmainak reviewed Sep 18, 2021

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

jasmainak reviewed Sep 19, 2021

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

ntolley added 9 commits September 21, 2021 11:17

Initial addition of probability argument to drives API

0e87125

Rename seedcore to event_seed, add no connection warning

ec58ade

Update examples

91861d2

Add tests for probability argument

7311998

Fix test failure with no connection warning

88f81a7

Update probability docs with dict argument description

430fdf7

Fix docstring and full match statements in tests

ebd5d03

Better test explanation

f65f8a6

Fix random connection seeding

14dabf9

ntolley force-pushed the drive_probability branch from ba0bd93 to 14dabf9 Compare September 21, 2021 15:20

ntolley and others added 4 commits September 21, 2021 15:25

Better random seeding documentation

d0bca52

better use of context manager for tests

7dd80da

Update hnn_core/network.py

de10b56

Co-authored-by: Mainak Jas <[email protected]>

Update whats_new.rst

ec6f3b8

rythorpe merged commit 1c2662c into jonescompneurolab:master Sep 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Add probability arguments to drives API #416

[MRG] Add probability arguments to drives API #416

ntolley commented Aug 20, 2021 •

edited

Loading

ntolley commented Aug 20, 2021

jasmainak commented Aug 20, 2021

jasmainak commented Aug 20, 2021

codecov-commenter commented Aug 20, 2021 •

edited

Loading

jasmainak commented Aug 23, 2021

ntolley commented Aug 23, 2021

jasmainak commented Aug 25, 2021

ntolley commented Aug 25, 2021

rythorpe left a comment

jasmainak left a comment

ntolley Sep 17, 2021

jasmainak Sep 18, 2021

rythorpe Sep 17, 2021

ntolley Sep 19, 2021

rythorpe Sep 22, 2021

jasmainak commented Sep 18, 2021

jasmainak commented Sep 18, 2021

jasmainak commented Sep 22, 2021

rythorpe commented Sep 22, 2021

[MRG] Add probability arguments to drives API #416

[MRG] Add probability arguments to drives API #416

Conversation

ntolley commented Aug 20, 2021 • edited Loading

ntolley commented Aug 20, 2021

jasmainak commented Aug 20, 2021

jasmainak commented Aug 20, 2021

codecov-commenter commented Aug 20, 2021 • edited Loading

Codecov Report

jasmainak commented Aug 23, 2021

ntolley commented Aug 23, 2021

jasmainak commented Aug 25, 2021

ntolley commented Aug 25, 2021

rythorpe left a comment

Choose a reason for hiding this comment

jasmainak left a comment

Choose a reason for hiding this comment

ntolley Sep 17, 2021

Choose a reason for hiding this comment

jasmainak Sep 18, 2021

Choose a reason for hiding this comment

rythorpe Sep 17, 2021

Choose a reason for hiding this comment

ntolley Sep 19, 2021

Choose a reason for hiding this comment

rythorpe Sep 22, 2021

Choose a reason for hiding this comment

jasmainak commented Sep 18, 2021

jasmainak commented Sep 18, 2021

jasmainak commented Sep 22, 2021

rythorpe commented Sep 22, 2021

ntolley commented Aug 20, 2021 •

edited

Loading

codecov-commenter commented Aug 20, 2021 •

edited

Loading