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

extracellular times-recording unnecessary #428

Open
cjayb opened this issue Sep 6, 2021 · 8 comments
Open

extracellular times-recording unnecessary #428

cjayb opened this issue Sep 6, 2021 · 8 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@cjayb
Copy link
Collaborator

cjayb commented Sep 6, 2021

The fix in #427 also includes a removal of the arbitrary one-sample-mismatch between the length of the potential and dipole traces. The latter are one short because extra_scatter_gather is only called after a solver step. The solution was to set the initial state to 0 mV (which I suppose is debatable).

Presently _ExtracellularArrayBuilder creates an independent copy of h.t for every array attached to the network:

self._nrn_times = h.Vector().record(h._ref_t)

This is redundant, since h._ref_t is already recorded in network_builder.py:_simulate_single_trial(). One simple solution is to let arr._times = sim_data[0]['times'] in the below (parallel_backends.py:_gather_trial_data())

        # extracellular array
        for arr_name, arr in net.rec_arrays.items():
            # voltages is a n_trials x n_contacts x n_samples array
            arr._data.append(sim_data[idx]['rec_data'][arr_name])
            arr._times = sim_data[idx]['rec_times'][arr_name]

Do I get a +1 for this? And in the bigger picture, do we want keep having times embedded in each of the various output objects we create (dpl, rec_arr, CellResponse)?

@rythorpe
Copy link
Contributor

rythorpe commented Sep 6, 2021

Sounds good to me. Idk why we need redundant copies of times passed around if they are all the same.

@jasmainak
Copy link
Collaborator

agree there should only be one "master" times. Not sure if the initial state should be included? I think having times in the different objects is fine for convenience with plotting etc? We just make sure it's set only in _gather_trial_data so they are all equal. Could even have a test that checks for this

@jasmainak
Copy link
Collaborator

0.2 milestone @cjayb ?

@cjayb
Copy link
Collaborator Author

cjayb commented Sep 7, 2021

We just make sure it's set only in _gather_trial_data so they are all equal.

Yeah this seems like the easiest approach that's still convenient (plotting). I hesitate because it causes an unnecessary copy, but times isn't something to worry about, I guess.

Not sure if the initial state should be included?

True, especially since our initial state is somewhat poorly defined.

0.2 milestone @cjayb ?

I have a suspicion this might balloon a bit, possibly interact with the pending discussion on initial 'transients' etc. Let's leave this Issue open for now and see what that other topic churns up?

@jasmainak
Copy link
Collaborator

sounds good, seems a bit independent of the 'transients' conversation to me but I let you decide

@jasmainak jasmainak added the good first issue Good for newcomers label Feb 10, 2022
@raj1701
Copy link
Contributor

raj1701 commented Mar 7, 2023

Can I work on this issue @cjayb @jasmainak @rythorpe ?

@cjayb
Copy link
Collaborator Author

cjayb commented Mar 7, 2023

Hey Raj, I'm afraid I'm quite useless here as I haven't looked at the code since last year :( I don't know the relevance of this Issue, but it is labeled as a good first issue, @jasmainak @rythorpe ?

@jasmainak
Copy link
Collaborator

One issue at a time @raj1701 ... let's first see your other PRs merged

@ntolley ntolley assigned ntolley and dylansdaniels and unassigned ntolley Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants