[MRG] ENH: calcium dynamics

[jasmainak]

Closes #384 #111
the API has moved since #275 . Starting a new PR.

Todos

- [x] update examples
- [ ] replace dpl.txt
- [x] update param files from https://www.dropbox.com/sh/ynpcwiw3t627ox8/AADkcq-AobmaATiowXKoVm4Wa?dl=0

Edit: Saving updated examples for separate PR. The goal of this PR is to just introduce a function to load the calcium model.

[ntolley]

I suppose the parallel backends test will fail until we replace dpl.txt with the updated simulation.

What is left for this PR? I am guessing that some of the values in the examples will need to be updated based on @samikane's tests. Anything else that I am missing?

[jasmainak]

yeah something really funky happened to the examples. What needs to be done to fix them? Any hints @samikane ?

Maybe we could have an option to switch back and forth between the old and new model. But it will depend on how substantially the examples need to change. I added a list of todos in the PR description.

[ntolley]

Something that has come to mind is the naming convention for this model. The publication for this is a WIP, so it does not belong under the "published models" section. One idea would be to name it calcium_model() for now, and pugilese_202?_model() whenever the paper is published. We can use a deprecation warning for the name change of the function, as it seems unnecessary to carry around redundant model names.

[jasmainak]

another option is to make calcium_model a private function and call it from default_model since it represents our "latest understanding". The older default model is still available as jones_2009_model. Wdyt?

[ntolley]

Very excited to see what chaos comes out of the examples now that I've frivolously swapped out the L5 pyramidal cells. I'll start thinking about which examples make sense to update, as something like plot_simulate_somato.py assumes the Jones 2009 model.

[ntolley]

@jasmainak let me know what you think of this solution for setting the calcium mechanism. I'm using the partial function from functools because it seems that nesting a function inside a function was preventing pickling of the network:

I'm always happy for an opportunity to show off obscure python internals 😄, but maybe there's a simpler solution...

[jasmainak]

Looks reasonable to me!

On Thu 24 Jun 2021 at 19:43, Nick Tolley ***@***.***> wrote: @jasmainak <https://github.com/jasmainak> let me know what you think of this solution <1fbb2ef> for setting the calcium mechanism. I'm using the partial function from functools because it seems that nesting a function inside a function was preventing pickling of the network: [image: image] <https://user-images.githubusercontent.com/55253912/123345315-cb851400-d523-11eb-9a73-16123bcf4c43.png> I'm always happy for an opportunity to show off obscure python internals 😄, but maybe there's a simpler solution... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#333 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADY6FIWHRFBYWZYXM7OKEA3TUO7IXANCNFSM443J3LFA> .

-- Sent from my iPhone

[ntolley]

Going through the examples, I am thinking that the add_drives_from_params argument really doesn't make sense in this case now that we are updating these values. Two possibilities, we can manually define the ERP in every function. Alternatively, we can create a default_erp function that lives in network_models.py and use that to quickly add the normal ERP.

I'm thinking it could be of a similar form to the one defined in plot_simulate_beta,py

[ntolley]

I've updated the examples that seem to make sense from the param files. Besides the evoked response parameters, most examples were fairly minimally impacted. I'll do another read through tomorrow to make sure I am not missing anything.

One update I have an issue with is the gamma example. The updated parameter values produce a gamma oscillation that is a much lower frequency (~30 Hz) compared to the 50-60 Hz oscillation in the current examples. I might sweep through a few parameter values to get it closer to the original example.

I think there's a good bit left to discuss for this PR, but this is turning out a bit less painful than I first anticipated.

[ntolley]

Ready for some more reviews! I am still looking to confirm the new parameter values, but everything else is in good shape.

I'm not really sure about how to update dpl.txt so I may need some help with that. I imagine that we can really only do it when the PR is just about to be merged?

(Also I have commented out the comparison tests temporarily to make sure the remaining tests are legitimately passing)

[codecov-commenter]

Codecov Report

Merging #333 (1315555) into master (08daf20) will increase coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head 1315555 differs from pull request most recent head 0e345f4. Consider uploading reports for the commit 0e345f4 to get more accurate results

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   89.91%   90.20%   +0.28%     
==========================================
  Files          16       16              
  Lines        2965     3000      +35     
==========================================
+ Hits         2666     2706      +40     
+ Misses        299      294       -5     

Impacted Files	Coverage Δ
hnn_core/network.py	91.91% <ø> (ø)
hnn_core/__init__.py	100.00% <100.00%> (ø)
hnn_core/cells_default.py	100.00% <100.00%> (+2.24%)	⬆️
hnn_core/network_models.py	100.00% <100.00%> (ø)
hnn_core/params.py	90.15% <0.00%> (+1.18%)	⬆️

---

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 08daf20...0e345f4. Read the comment docs.

[jasmainak]

The gamma example looks a bit off though. The waveform is at the wrong frequency ...

[ntolley]

The gamma example looks a bit off though. The waveform is at the wrong frequency ...

#333 (comment) yeah I definitely agree. I'm starting to get the sense that we're not quite ready for an update to every example. One option would be to distribute the updated model, and hold off on making it the new standard until we're confident with the parameters.

Specifically leave default_network() intact, and make _calcium_model() public.

[jasmainak]

Yeah I tend to agree. I am beginning to think it may not be a good idea to have default_model be a moving target. Imagine someone runs the same script using HNN-core in 2 years and they get completely different looking results!

[jasmainak]

I suggest getting rid of default_network and just have users explicitly call the models jones_2009_model, law_2021_model and calcium_model

[ntolley]

Imagine someone runs the same script using HNN-core in 2 years and they get completely different looking results!

And that someone would very likely be me 😄. I like the proposed changes and agree that having default_network changing constantly is rather troublesome. If there is a better understanding of the model we'd like to share, that can be done in the examples and not the API.

To get this PR moving, how about we just get calcium_model and the associated tests in a good condition, and leave the updated examples for a separate PR once we have the parameters sorted out?

[jasmainak]

Yeah, I like that plan. In fact, what I'd really love to see is have #77 merged and point users to that function and say they can use it whenever there is a new model. Maybe the calcium model can even be an example for #77

[ntolley]

@jasmainak I have implemented the changes discussed. Specifically, I have removed default_network entirely, and have replaced it with jones_2009_model in all examples/tests.

Lastly, reverted the examples back to their old parameter values. I left in the add_erp_drives_to_jones_model function and its associated tests for when we remove add_drives_from_param, but have kept it out of the examples.

[jasmainak]

@ntolley looks good, can you change the status to MRG if it's good to go by you? Also, do you think you could squash the commits into the "highlights" of say 10 commits, so we keep a clean history?

$ git rebase -i HEAD~30

to rebase last 30 commits for example, and then just edit to squash adjacent commits

[ntolley]

Sure thing! I'll flip it to [MRG] once I push the squashed commits

[ntolley]

@jasmainak I left the original commit messages for transparency, but were you looking for them to be edited down to one-liners?

[jasmainak]

No that's fine! I'll let @cjayb and @rythorpe take one look before merge

[rythorpe]

Looks good @ntolley! I've been absent from a lot of the conversation on this PR but all the important pieces look clean and complete. On a side note, I'm coming around to agreeing with your decision to remove default_network() as it makes the assumptions in our code crystal clear.

[jasmainak]

Thanks @ntolley ! And thanks @rythorpe for the review 🍻