Arbor cable cell exporter and backend #393
Conversation
…ted simplecell and l5pc examples)
bluepyopt/ephys/create_acc.py
Outdated
| # Define BluePyOpt to Arbor region mapping (relabeling locations to SWC convention) | ||
| # Remarks: | ||
| # - using SWC convetion: 'dend' for basal dendrite, 'apic' for apical dendrite | ||
| # - myelinated is unsupported in Arbor |
There was a problem hiding this comment.
Can we add support for myelinated in Arbor? Do you know how these section are defined?
There was a problem hiding this comment.
I'm not completely sure where this is used (it doesn't seem to be in l5pc). A quick search finds it in several .hoc files in examples/stochkv. Does somebody else have insights on this?
There was a problem hiding this comment.
'myelinated' is a sectionlist we're using in our latest models. It's a cylinder attached to the end of the AIS to prevent boundary effects. It doesn't have channels but just represent a myelinated chunk of axon
There was a problem hiding this comment.
Thanks @wvangeit for the explanation. I assume AIS means axon initial segment. Is there an example/morphology with myelinated? Otherwise, I'd suggest to throw an exception for now until we can properly test that in Arbor.
There was a problem hiding this comment.
We don't have this in open source models yet. But it's fairly simple code though. It's not specified in the morphology file itself, but used in the replace_axon function when constructing the model:
sim.neuron.h.execute('create myelin[1]', icell)
icell.myelinated.append(sec=icell.myelin[0])
icell.all.append(sec=icell.myelin[0])
icell.myelin[0].nseg = 5
icell.myelin[0].L = 1000
icell.myelin[0].diam = diams[count-1]
icell.myelin[0].connect(icell.axon[1], 1.0, 0.0)
There was a problem hiding this comment.
Tested - it works with your code above added to replace_axon. I've added the necessary changes to c0af192, but they're currently commented out as I have no way to figure out if the myelin section exists without crashing in Neuron.
There was a problem hiding this comment.
not sure, but i wonder if hasattr():
https://www.w3schools.com/python/ref_func_hasattr.asp
would work on icell?
There was a problem hiding this comment.
That works on myelin and the len(self.icell.myelin) == 1 looks like a bug given that NEURON crashes, complaining that the section was deleted upon accessing e.g. self.icell.myelin[0]. I've added safe support for myelin now in 2c67258.
There was a problem hiding this comment.
Let's look at this in a separate issue.
bluepyopt/ephys/create_acc.py
Outdated
| # Define BluePyOpt to Arbor region mapping (relabeling locations to SWC convention) | ||
| # Remarks: | ||
| # - using SWC convetion: 'dend' for basal dendrite, 'apic' for apical dendrite | ||
| # - myelinated is unsupported in Arbor |
There was a problem hiding this comment.
'myelinated' is a sectionlist we're using in our latest models. It's a cylinder attached to the end of the AIS to prevent boundary effects. It doesn't have channels but just represent a myelinated chunk of axon
|
Ok, thank you, that already looks nice. |
…or), simplecell_model.py simplified and consistent with notebook
|
Thank you @wvangeit for the review! The tests should pass now. Please let me know if there are open issues. I'll have to prepare an example comparing Arbor with NEURON simulation output (analogous to the Allen tutorial mentioned above). |
|
@AurelienJaquier @DrTaDa , after merging master into this PR, some test failed. But I think it's something we recently added? |
|
@wvangeit Indeed, I hardcoded a value for the a test for CMA but it seems that I should use a rtol or atol on the test instead of an equality: |
…ACC-export, demo of simulating protocols in Arbor on simple cell example and validation with Neuron
The validation of Arbor simulation results with those obtained from Neuron is done for the simple cell example in |
… arbor_integration
…on for GUI-compatibility
…alogue to JSON, global ion properties, create_acc with axon replacement simplified
|
@lukasgd could you please restore this PR to the state after @anilbey latest changes, or create new PR based on that state. It seems you have overwritten the history and squashed some commits. We should keep the history of this branch intact. |
|
Hi @lukasgd
Thanks for taking care of the code but we have to respect the principles of this repository (and, in general, OSS development expects that the community respect the maintainers process). The PR author makes the suggestions and the maintainers decide whether to apply them. Each organisation has different ways of working. You cannot enforce them to change their principles. Otherwise people could not have maintained large repositories if each PR author introduced their rules.
Here making it a function makes a difference. It prevents side-effect caused by the global scope import. Before it was in the global scope. It means it was getting executed whenever someone imports something from the module. E.g. if someone imports
Having an interface that both hoc and acc adheres to would be good indeed. But the code was not there. Before making changes, first this code must be readable, maintainable, unit testable. I had made some changes to begin us along that path, but by squashing them, this has made the maintenance side of things more difficult - for instance the CI is failing, and it's difficult to track down why.The last state of the repository I left was "good enough" for me to merge for the time being. It means for now this much structuring is enough for the code to be readable and maintainable. In the future when new features come and BluePyOpt becomes more complex, more refactorings will be needed but for now it's okay. This being said, future improvements are welcome but please respect the style of the repository and the maintainer's time. Thus, please strive for:
Although the unit test coverage increased, of course it is possible that my changes might have introduced bugs. If there's a bug, ideally the tests should be catching it, not visual inspection. That's why the unit test coverage must be high. Please inform me on the bug and how to reproduce it so that I can test it and fix it.
In the future I would like to apply the same exception raising strategy (if it's a python built-in exception like type error then use it. If it's something very specific, then define it) in the other modules as well. However it's not within the scope of this already large PR.
It's the maintainers who decide this. Some large repositories even demand machine readable commit messages that includes certain keywords and structure. When I make a PR to a large code-base they have much stricter rules than this and I have to abide their rules because it's their system. By the way, our small commits idea is not our invention. It comes from the literature and it's widely applicable. Kent Beck, Martin Fowler, Bob Martin etc. have expressed this idea of small, testable, revertible steps (commits) in various books, articles and talks. They are interesting resources. I once again would like to thank you for your valuable contributions. Apologies if this comment appears overboard. I wanted to clarify our responsibilities to collaborate better. I created #422 from the previous state of code. The tests pass there. They currently fail here with the newest changes. I think #422 is ready to be merged. You mentioned a bug, what is it? With that fix I can merge #422. Is there something critical you think should be added to #422? For the newer features of course we can have new PRs. |
There was a problem hiding this comment.
Hopefully the final review :)
Overall good.
@brenthuisman, @thorstenhater please see to the feedback.
| point_loc=param.point_loc, | ||
| value=cls._param_value(param)) | ||
| else: | ||
| raise CreateAccException( |
There was a problem hiding this comment.
TypeError not CreateAccException.
| def __init__(self, message): | ||
| """Constructor""" | ||
|
|
||
| super(CreateHocException, self).__init__(message) |
There was a problem hiding this comment.
Not needed, nothing special is done in init.
|
Hi @anilbey thanks for the review! I'm probably not going to get round to addressing it before the break, so I plan to get started first thing next year. |
|
Thanks for the review @anilbey. Please double-check the Arbor import mechanism at Also, the L5PC |
|
Thanks @lukasgd for the changes. I am okay with the import mechanisms.
I see, this happens often with NEURON. One test leaves some global state and the other tests are affected by that state. In tests using NEURON we tolerate it because we know that it is due to NEURON's design. I guess it's also ok for Arbor tests to differ in precision at the 6th digit when not isolated. This issue can be more important for Arbor, than for us. I.e. if it is not expected that independent runs affect each other, maybe it is something to look at. I don't know. Like @brenthuisman I will also leave for holidays, let's continue finishing it next year. |
|
Thanks for the feedback, @anilbey. The test suite is fixed now - the altered coordinates in the L5PC morphology with axon replacement instantiated with Neuron were introduced with a version bump of Neuron (v8 to 9) causing The main remaining point now that I'm aware of is to integrate the recent updates from the master branch. If the above things need any more work, please let me know. |
|
I'm uploading a proposal for merging master here. There are few things that I came across. First, setting tox' minversion to 4 causes this error for me dev/BluePyOpt$ tox
ERROR: venv '.tox' in /home/lukasd/src/arbor/dev/BluePyOpt would delete project
Traceback (most recent call last):
File "/home/lukasd/src/arbor/dev/venv/bin/tox", line 8, in <module>
sys.exit(cmdline())
File "/home/lukasd/src/arbor/dev/venv/lib/python3.8/site-packages/tox/session/__init__.py", line 44, in cmdline
main(args)
File "/home/lukasd/src/arbor/dev/venv/lib/python3.8/site-packages/tox/session/__init__.py", line 69, in main
exit_code = session.runcommand()
File "/home/lukasd/src/arbor/dev/venv/lib/python3.8/site-packages/tox/session/__init__.py", line 186, in runcommand
provision_tox_venv = self.getvenv(self.config.provision_tox_env)
File "/home/lukasd/src/arbor/dev/venv/lib/python3.8/site-packages/tox/session/__init__.py", line 137, in getvenv
raise tox.exception.ConfigError("envdir must not equal toxinidir")
tox.exception.ConfigError: ConfigError: envdir must not equal toxinidirI didn't set any of these dirs - how do I fix this? Regarding the updates from master I found the following:
|
|
@lukasgd about tox, I usually have this error when I have installed a tox with version < 4, while having |
|
Thanks, @AurelienJaquier - indeed, upgrading tox solved it. I've uploaded new validation runs here. Note that I've used different random parameter samples compared to the ones before, but the findings remain the same. |
|
The tests should pass now as far as my internet connection allows to check. From my side, the PR is ready. Is there anything more that needs to be addressed? |
|
Thank you @lukasgd, the tests are indeed working. @thorstenhater @brenthuisman , one last check, you're ok with the current implementation? Then I'll merge the PR. |
|
If you are happy and all breakage is addressed, you have my blessing :) Thanks everyone! |
|
Finally merged :-) Thank you for all the hard work @lukasgd et al. |
|
Thank you for accepting it @wvangeit and thanks to everyone for their contribution and guidance! |
This is a small PR to track updates in the BluePyOpt API (cf. BlueBrain/BluePyOpt#393) compared to the v0.8 release.
I'm working on an Arbor cable cell exporter for BluePyOpt optimized cell models (related to arbor-sim/arbor#1839) and looking for feedback here.
I've added a
create_accmodule analogous tocreate_hocinbluepyopt.ephys. It works with thesimplecellandl5pcexamples. Usinggenerate_acc.pyin the corresponding example folder, you can output a cell with optimized parameters to 3 files (plus morphology) - a JSON file for metainformation that references a morphology file as well as two ACC files - one for the decor and one for the label dictionary. The output of runningis
with file contents
{ "cell_model_name": "simple_cell", "produced_by": "Created by BluePyOpt(1.12.5) at 2022-04-06 18:17:54.383572", "morphology": "simple.swc", "label_dict": "simple_cell_label_dict.acc", "decor": "simple_cell_decor.acc" }For the
l5pcthe output is more comprehensive (obtained analogously withBluePyOpt/examples/l5pc/generate_acc.py).with an analogous JSON and the same label_dict as above, but different decor,
Once a cable cell is constructed in Arbor, it's possible to write it to ACC (
arbor.write_component) and visually inspect it in the Arbor GUI. For thel5pcexample, the ACC output is in l5pc_full_no_mechs.zip (mechanisms removed from decor) and looks as follows:I'm also working on Arbor simulation examples that use these two exported cells.
For the code below, I'd like to mention that I slightly refactored
create_hocby adding two new function calls inside thecreate_hocfunction, so that code from the existing module can be reused increate_acc(using_generate_template_params) and control flow is similar between the two (create_hoc._read_template/create_acc._read_templates). The logic/instructions increate_hocare unchanged.The tests in
test_create_hoc.pyare still succeeding and I've added analogous ones for the JSON/ACC output intest_create_acc.py.If you have feedback, I'm interested to hear from you.