Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove legacy drive conventions and functions #393

Closed
rythorpe opened this issue Jul 7, 2021 · 5 comments
Closed

remove legacy drive conventions and functions #393

rythorpe opened this issue Jul 7, 2021 · 5 comments

Comments

@rythorpe
Copy link
Contributor

rythorpe commented Jul 7, 2021

I think this merits it's own issue. Quoting @cjayb,

OK, drives.py:drive_event_times is the remains of feeds.py, i.e., deprecated (used to be called feed_event_times). The test test_drives.py:test_external_drive_times is basically testing that the expected "default" drives are read from the params-file, and that the gid-assignments are 'correct' insofar as they reproduce default GUI output.

That can all go, though I suppose we should immediately open a follow-up PR to implement tests of drives.py:_drive_cell_event_times!

I'd also like to point out that I think this hack might be worth removing in this PR

# this is needed to keep the drive GIDs identical to those in HNN,
# e.g., 'evdist1': range(272, 542), even when no L5_basket cells
# are targeted (event times lists are empty). The logic in
# _connect_celltypes is hard-coded to use these implict gid ranges
if self._legacy_mode:
# XXX tests must match HNN GUI output
target_populations = list(self.cell_types.keys())

The tricky part is here for cases when all weights are zero (at least is used to be a problem before this PR...):

pos_idx = src_gid - net.gid_ranges[_long_name(src_type)][0]
# NB pos_dict for this drive must include ALL cell types!
nc_dict['pos_src'] = net.pos_dict[
_long_name(src_type)][pos_idx]

Originally posted by @cjayb in #383 (comment)

@rythorpe
Copy link
Contributor Author

rythorpe commented Jul 7, 2021

OK, drives.py:drive_event_times is the remains of feeds.py, i.e., deprecated (used to be called feed_event_times). The test test_drives.py:test_external_drive_times is basically testing that the expected "default" drives are read from the params-file, and that the gid-assignments are 'correct' insofar as they reproduce default GUI output.

That can all go, though I suppose we should immediately open a follow-up PR to implement tests of drives.py:_drive_cell_event_times!

I totally agree, let's get rid of drives.drive_event_times() in favor of building tests off of drives._drive_cell_event_times.

I'd also like to point out that I think this hack might be worth removing in this PR

# this is needed to keep the drive GIDs identical to those in HNN,
# e.g., 'evdist1': range(272, 542), even when no L5_basket cells
# are targeted (event times lists are empty). The logic in
# _connect_celltypes is hard-coded to use these implict gid ranges
if self._legacy_mode:
# XXX tests must match HNN GUI output
target_populations = list(self.cell_types.keys())

[Updated as of this comment] I'm actually in favor of making

target_populations = list(self.cell_types.keys())

standard practice (not just legacy) because it ensures that artificial drive cells are created irrespective of the location of the drive or specific cell types within the network. This seems like good practice as it simplifies the logic for how/when an artificial cell gets created.

@cjayb
Copy link
Collaborator

cjayb commented Jul 9, 2021

Hmm, interesting! I think this makes a lot of sense after your work in #383 Either way, removes the need for the legacy-flag

@jasmainak
Copy link
Collaborator

@rythorpe does something remain unaddressed in this PR after #369 ?

@rythorpe
Copy link
Contributor Author

Nope.

@rythorpe
Copy link
Contributor Author

Any residual issue(s) were addressed in #431.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants