fix arbor tests with latest arbor version #498
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #498 +/- ##
=======================================
Coverage 87.36% 87.37%
=======================================
Files 50 50
Lines 4346 4349 +3
=======================================
+ Hits 3797 3800 +3
Misses 549 549 ☔ View full report in Codecov by Sentry. |
bluepyopt/ephys/morphologies.py
Outdated
| @@ -307,6 +307,8 @@ def load(morpho_filename, replace_axon): | |||
| morpho = arbor.load_component(morpho_filename).component | |||
| elif morpho_suffix == '.swc': | |||
| morpho = arbor.load_swc_arbor(morpho_filename) | |||
| # turn loaded_morphology into morphology type | |||
| morpho = arbor.morphology(morpho.segment_tree) | |||
There was a problem hiding this comment.
This should be easier by using
| morpho = arbor.morphology(morpho.segment_tree) | |
| morpho = morpho.morphology |
There was a problem hiding this comment.
Done in latest commit
bluepyopt/ephys/protocols.py
Outdated
| @@ -631,7 +636,7 @@ def instantiate_recordings(self, cell_model, use_labels=False): | |||
| """Instantiate recordings""" | |||
|
|
|||
| # Attach voltage probe sampling at 10 kHz (every 0.1 ms) | |||
| for i, rec in enumerate(self.recordings): | |||
| for _, rec in enumerate(self.recordings): | |||
There was a problem hiding this comment.
Why use enumerate just to drop the index we just added?
There was a problem hiding this comment.
Keeping the index in the latest commit
bluepyopt/ephys/protocols.py
Outdated
| @@ -648,7 +653,9 @@ def instantiate_recordings(self, cell_model, use_labels=False): | |||
|
|
|||
| cell_model.probe('voltage', | |||
| arb_loc.ref if use_labels else arb_loc.loc, | |||
| frequency=10) # could be a parameter | |||
| "0", # tag: default is '0' | |||
There was a problem hiding this comment.
Better:
| "0", # tag: default is '0' | |
| f"probe-{i}", |
and keep the index above. Sampling then needs to reference that tag.
Alternatively, we can label by the location-expression and assert that
those are unique.
There was a problem hiding this comment.
Done in latest commit.
| @@ -10,7 +10,7 @@ | |||
| {%- endif %} | |||
| {%- else %} | |||
| {%- for param in params %} | |||
| (default ({{ param.name }} {{ param.value }})) | |||
| (default ({{ param.name }} {{ param.value }} (scalar 1.0))) | |||
There was a problem hiding this comment.
I won't check all of those in detail, but this is correct for a fixed value.
If the tests are green, I'd assume all are correct.
There was a problem hiding this comment.
The tests are green
There was a problem hiding this comment.
I'd recommend to additionally check the major examples' notebooks qualitatively by inspection (not everything is run as part of the test suite).
There was a problem hiding this comment.
As said before using Arbor to write these might be an easier option.
There was a problem hiding this comment.
Is there a function that can write these kind of files?
There was a problem hiding this comment.
Yes, there's versions for full cable cells or just parts thereof.
https://docs.arbor-sim.org/en/latest/fileformat/cable_cell.html#parsable-arbor-components-and-meta-data
In Python, you'd construct a cable cell (or the parts you're interested in) and call arbor.write_component(thing, filename)
https://docs.arbor-sim.org/en/latest/python/cable_cell_format.html#reading-and-writing-arbor-components
There was a problem hiding this comment.
That would basically mean to turn this template into code at the end of the ACC exporter by instantiating Arbor objects that are then serialized again. Could be an option - I guess it comes down to how much control you need over the export vs. learning the s-expression syntax. The original design of BluePyOpt was to make model exporting configurable (overridable by the user) through templates, hence I chose this option.
In my view, most of the complexity in the ACC exporter comes from converting BluePyOpt's frontend representation to one suitable for Arbor. That last step of dumping the decor is less complicated, but requires some understanding of the format.
I don't want to take any particular side, though. Would only suggest that if you plan to make major changes to the ACC exporter, maybe a good idea would be to do that in a separate PR.
There was a problem hiding this comment.
Yes, I'd support splitting this. One straight port that can be verified and a lateral move to another scheme.
The original design of BluePyOpt was to make model exporting configurable (overridable by the user) through templates, hence I chose this option.
This is why I as an outsider should have an opinion here :D
|
Picking one of the examples from the test suite (arbor-component
(meta-data (version "0.9-dev"))
(decor
(paint (region "soma") (membrane-capacitance 0.01 (scalar 1.0)))
(paint (region "soma") (density (mechanism "default::hh" ("gnabar" 0.10299326453483033) ("gkbar" 0.027124836082684685))))))should be produced by import arbor as A
d = A.decor()
d.paint('"soma"', cm=0.01)
d.paint('"soma"', A.density('default:hh', gnabar=0.10299326453483033, gkbar=0.027124836082684685))
A.write_component(d, 'test.acc') |
|
Thanks for your help @lukasgd @thorstenhater , I think this PR is good to be merged. |
dropped support for python 3.8 because latest arbor does not support it and python 3.8 will reach end of life in one month.