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

Arbor cable cell exporter and backend support in BluePyOpt #1959

Merged
merged 31 commits into from
Oct 25, 2022

Conversation

lukasgd
Copy link
Contributor

@lukasgd lukasgd commented Aug 24, 2022

This is functionality to support Arbor simulations with cells optimized with Neuron in BluePyOpt as implemented in BlueBrain/BluePyOpt#393 and related to #1839.

It includes exported versions and example scripts for the simplecell and l5pc examples from BluePyOpt. The format is detailed in the referenced PR. The functionality added to Arbor includes pruning a particular tag from a morphology (going through a segment tree) and calculating an approximation for the radius of the axon replacement.

Closes #1839
Closes #1221

Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, a few comments

  • Some of the utilities seems overly specific, eg media_distal_radii
    • Can we write them in terms of the public API and shuffle them to an external project?
    • Or -- failing that -- generalise them?
  • Please add tests and documentation
  • Consider rebuilding prune_tags on Allow editing morphologies #1957, which does similar things.

arbor/include/arbor/morph/morphology.hpp Show resolved Hide resolved
arbor/include/arbor/morph/segment_tree.hpp Outdated Show resolved Hide resolved
arbor/include/arbor/morph/segment_tree.hpp Outdated Show resolved Hide resolved
arbor/morph/morphology.cpp Outdated Show resolved Hide resolved
python/example/single_cell_bpo/l5pc/Readme.md Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
@thorstenhater
Copy link
Contributor

I added the two related issues to be closed

@lukasgd
Copy link
Contributor Author

lukasgd commented Sep 15, 2022

All requests should be addressed, prune_tag and median_distal_radii are removed (see comments for details), split_at fixed, tests and documentation added.

@lukasgd
Copy link
Contributor Author

lukasgd commented Sep 15, 2022

Changed the split_at implementation again to avoid redundant loops on large models. I've changed the node predicate as well to a segment id predicate - the node struct contains the parent ID in the new subtree, which seems not very useful to filter on.

thorstenhater
thorstenhater previously approved these changes Sep 16, 2022
Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nitpicks left!

@@ -0,0 +1,13 @@
# BluePyOpt layer-5 pyramidal cell with axon-replacement

This example was created in an environment with the `bluepyopt` package installed (see [installation instructions](https://github.com/BlueBrain/BluePyOpt#installation)). A cell model with parameters as published by [Markram et al., "Reconstruction and simulation of neocortical microcircuitry", Cell 163.2 (2015): 456–492](http://www.cell.com/abstract/S0092-8674%2815%2901191-5) (see [L5PC.ipynb](https://github.com/BlueBrain/BluePyOpt/blob/master/examples/l5pc/L5PC.ipynb)) can be exported with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last question on this: the licensing is unproblematic I hope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to forward this to @wvangeit - is it ok to add this exported L5PC model as an example as described here to the Arbor repo?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this falls under the BSD license of the entire repo? For me it's ok if you keep the reference to the original notebooks.
Are you storing any of the MOD files of the L5PC, remember that these have their own licenses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I didn't store any of the L5PC mechs in Arbor, but rather took the ones from the built-in BBP catalogue. This is also the ones that I used in the Arbor-Neuron validation (and actually I have even tested the K_Tst and K_Pst with Neuron to make sure the difference between the simulators we found is not due to the NMODL code).

python/example/single_cell_bpo_simple.py Outdated Show resolved Hide resolved
python/example/single_cell_bpo_l5pc.py Outdated Show resolved Hide resolved
python/example/single_cell_bpo_l5pc.py Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
arbor/morph/segment_tree.cpp Show resolved Hide resolved
arbor/morph/segment_tree.cpp Outdated Show resolved Hide resolved
@thorstenhater
Copy link
Contributor

@lukasgd could you

  1. Merge master
  2. Write a nice commit message
  3. Squash/merge

?

@lukasgd lukasgd changed the title Arbor cable cell exporter support in BluePyOpt Arbor cable cell exporter and backend support in BluePyOpt Oct 14, 2022
Copy link
Contributor

@brenthuisman brenthuisman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the tutorial, looks good! Nice use of the GUI!

doc/concepts/decor.rst Show resolved Hide resolved
doc/concepts/labels.rst Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Show resolved Hide resolved
thorstenhater
thorstenhater previously approved these changes Oct 18, 2022
Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, nothing serious. Rest is good.

doc/tutorial/network_ring_gpu.rst Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
doc/tutorial/single_cell_bluepyopt.rst Outdated Show resolved Hide resolved
@lukasgd
Copy link
Contributor Author

lukasgd commented Oct 18, 2022

Thanks for the feedback. @brenthuisman, I've resolved all points I've addressed in 622f204. Feel free to reopen any if you'd like further changes.

@lukasgd
Copy link
Contributor Author

lukasgd commented Oct 24, 2022

I've swapped the label-dict and decor arguments to the cable_cell constructor and in the returned values of read_acc. @brenthuisman, is there anything more from your review that needs to be addressed?

Copy link
Contributor

@brenthuisman brenthuisman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@thorstenhater
Copy link
Contributor

@lukasgd would you like to write the commit message and merge the whole affair?

@thorstenhater
Copy link
Contributor

The test failure is not critical, it's our old friend; should be gone in master

@lukasgd lukasgd merged commit da8450e into arbor-sim:master Oct 25, 2022
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

Successfully merging this pull request may close these issues.

Single cell parameter optimization Library-provided parameterized cell recipe
4 participants