Expose diverse probes in Python API. #1225

halfflat · 2020-11-09T22:13:35Z

Major changes in Python library and documentation:

Add global state (immutable after initialization) for the Python module that
manages the mapping between probe metadata types and the method by which the
corresponding sample data can be collected and presented as Python objects.
This is necessary for decoupling the implementation of the simulation wrapper
from that of the various cable cell probe types.
Wrap each of the C++ cable cell probe types with a Python function that
returns a probe_info object.
Add code for converting and accumulating C++ probe metadata and sample
data, registered in the module global state against the metadata type info.
Make the arbor.simulator object responsible for recording all spike and
sample data, with corresponding new methods.
Use NumPy arrays and structured datatypes for returning spike and
sample data.
Place Python schedule proxies under an abstract interface so that consumers
of the proxies can be made generic over schedule types.
Add unit tests for the arbor.simulator class and its new functionality,
including distributed spike collection.
Rework all Python API documentation concerning probes, sampling, and spike
recording; add information on newly exposed cable cell probe addresses.
Add new python example single_cell_recipe.py which is a generic recipe
version of single_cell_model.py.
Adjust other code in the wrapper and examples to accomodate these changes.

Minor changes in Python library:

Add equality tests for arbor.location and arbor.cable Python classes.
Use the py:: namespace shorthand more often in the Python wrapping sources.
Add an implicit conversion for a pair of numbers to arbor.cell_member, so
that e.g. probe ids can be specified simply as (gid, index).
Add an implicit conversion from a tuple to arbor.mpoint so that a
morphological point can be specified simply as (x, y, z, radius).
Modify the interface to arbor.regular_schedule so that the overloads
correspond more closely to the C++ API. In particular,
arbor.regular_schedule(dt) now describes a schedule with times 0, dt, 2*dt,
etc.

Minor changes in C++ library:

Change test in FVM lowered cell implementation and exception message for
bad_source_description: assert number of detectors is exactly the number
of sources promised by the recipe.
Add comment describing probe_metadata object in sampling.hpp which
asserts that the type and value of probe-specific metadata completely
determines the correct interpretation of sampled data provided to a callback.

Implements #1160.

thorstenhater · 2020-11-10T07:16:57Z

Hi,

a short comment on the motivation would be helpful for me.

T

noraabiakar

This is great!
I have a few questions and suggestions. My main problem is with the probe/sampling docs. I don't think they're very clear. They probably should be extended, if not in this PR, then soon after.

doc/python/cable_cell.rst

doc/python/simulation.rst

python/cable_probes.cpp

python/identifiers.cpp

python/test/unit/test_simulator.py

halfflat · 2020-11-10T18:48:11Z

a short comment on the motivation would be helpful for me.

I totally forgot to link to the corresponding issue (#1160). I've added that to the description above.

doc/python/simulation.rst

python/simulation.cpp

python/test/unit/test_simulator.py

arbor/CMakeLists.txt

bcumming · 2020-11-11T15:45:51Z

bors try

bors · 2020-11-11T16:03:43Z

try

Build succeeded:

ci/gitlab/gitlab.com

bcumming

Nice work. There are a few requested changes.

bcumming · 2020-11-11T15:51:11Z

doc/cpp/cable_cell.rst

@@ -377,6 +377,8 @@ Overriding properties locally
   the morphology.


+.. _cable-cell-probes:


Generic probe-related documentation is missing from the concepts docs. Can we move the generic definitions/concepts there, then have the cpp and python docs refer to those and give only language-specific implementation and usage details?

Would this be the content that's been put under simulation? I'm not averse to this, but maybe we could make the concept docs a different task, as it's not exactly in scope for this PR, and would delay merging.

I don't know what you are referring to by "under simulation".

The issue here is that documentation has been created under Python API, but nothing added to concepts. The cpp and python docs might be simplified if a lot of the common part of that documentation is moved to concepts. So I think that it is actually in the scope of this PR.

If it is in a separate PR, then it should be done ASAP, otherwise it will be forgotten.

I mean, I added some text in the documentation of the simulation class that explained the probes and sampling. Is this suitable material for moving into Concepts?

python/example/network_ring.py

python/example/single_cell_recipe.py

bcumming · 2020-11-11T16:07:50Z

python/identifiers.cpp

+            "  index:   The cell-local index of the item.\n")
+        .def(py::init([](py::tuple t) {
+                if (py::len(t)!=2) throw std::runtime_error("tuple length != 4");
+                return arb::cell_member_type{t[0].cast<arb::cell_gid_type>(), t[1].cast<arb::cell_lid_type>()};


What happens with these casts if the user passes a tuple with non-convertible types, e.g.:

m = arbor.cell_member(("hello", 23.2))

You can force pybind11 to do the casting and checking for you by making the arguments to be std::tuple<arb::cell_gid_type, arb::cell_lid_type> instead of pybind11::typle.

If we do that with the current version of the PR, we get:

RuntimeError: Unable to cast Python instance to C++ type (compile in debug mode for details)

Is that sufficient?

Sufficient. I was hoping that it would just work! I always have to test.

bcumming · 2020-11-11T16:11:32Z

python/identifiers.cpp

        "Enumeration used to indicate which hardware backend to execute a cell group on.")
        .value("gpu", arb::backend_kind::gpu,
            "Use GPU backend.")
        .value("multicore", arb::backend_kind::multicore,
            "Use multicore backend.");

-    pybind11::enum_<arb::binning_kind>(m, "binning",
+    py::enum_<arb::binning_kind>(m, "binning",


I deliberately chose pybind11 as the namespace. By doing this we are now inconsistent (you are probably going to show me one spot where I used py instead of pybind11 now.

Yeah, there were multiple cases of using py:: instead of pybind11::, so I went with the shorter for consistency and brevity.

bcumming · 2020-11-11T16:27:45Z

python/pyarb.hpp

@@ -0,0 +1,84 @@
+#pragma once


How about putting this content in python/cable_probes.hpp, because this is really specific to probes/samplers.

It's probes and samplers, but not cable probes specifically.

Part of the motivation for this design is to keep the simulator code uncoupled from the cable probe definitions and wrappers. This way, the cable probe wrapper code is responsible for registering its metadata handlers, and the simulator code is responsible for using those handlers to process samples.

python/simulation.cpp

python/test/unit_distributed/test_simulator.py

python/pyarb.cpp

Major changes in Python library and documentation: * Add global state (immutable after initialization) for the Python module that manages the mapping between probe metadata types and the method by which the corresponding [sample data can be collected and presented as Python objects. This is necessary for decoupling the implementation of the simulation wrapper from that of the various cable cell probe types. * Wrap each of the C++ cable cell probe types with a Python function that returns a `probe_info` object. * Add code for converting and accumulating C++ probe metadata and sample data, registered in the module global state against the metadata type info. * Make the `arbor.simulator` object responsible for recording all spike and sample data, with corresponding new methods. * Use NumPy arrays and structured datatypes for returning spike and sample data. * Place Python schedule proxies under an abstract interface so that consumers of the proxies can be made generic over schedule types. * Add unit tests for the `arbor.simulator` class and its new functionality, including distributed spike collection. * Rework all Python API documentation concerning probes, sampling, and spike recording; add information on newly exposed cable cell probe addresses. * Add new python example `single_cell_recipe.py` which is a generic recipe version of `single_cell_model.py`. * Adjust other code in the wrapper and examples to accomodate these changes. Minor changes in Python library: * Add equality tests for `arbor.location` and `arbor.cable` Python classes. * Use the `py::` namespace shorthand more often in the Python wrapping sources. * Add an implicit conversion for a pair of numbers to `arbor.cell_member`, so that e.g. probe ids can be specified simply as `(gid, index)`. * Add an implicit conversion from a tuple to `arbor.mpoint` so that a morphological point can be specified simply as `(x, y, z, radius)`. * Modify the interface to `arbor.regular_schedule` so that the overloads correspond more closely to the C++ API. In particular, `arbor.regular_schedule(dt)` now describes a schedule with times 0, dt, 2*dt, etc. Minor changes in C++ library: * Change test in FVM lowered cell implementation and exception message for `bad_source_description`: assert number of detectors is exactly the number of sources promised by the recipe. * Add comment describing `probe_metadata` object in `sampling.hpp` which asserts that the type and value of probe-specific metadata completely determines the correct interpretation of sampled data provided to a callback.

* Add named arguments to cable probe address functions. * Add tests for successful construction, correct metadata for each cable probe address function. * Build arbor library with -fPIC to avoid relocation errors with Python lib. * Remove redundant line of old debug code in Python simulaiton test. * Add pointer from Python cable cell probes documentation to simulation class documentation for details regarding probes etc. * Use `where` instead of `locations` in documentation of Python cable cell probe address wrappers, so that it matches the keyword name in the wrapper. * Fix typos and misformats identified in PR review. * Clarify in Python cable cell probe docs that location expressions in probe addresses are interpreted in the context of the cell on which the probe is defined. * Add missing assertion in multiple probe per probe id metadata check in test_simulator.py. * Add `single_cell_recipe.py` to set of Python examples run in Travis test script.

* Add comment re: PYBIND11_NUMPY_DTYPE. * Remove debug prints from unit_distributed/test_simulator.py. * Add `ci=None` to seaborn plot in single_cell_recipe.py. * Rename: `py_simulation` -> `simulation_shim`; `py_spike_recording` → `spike_recording`.

halfflat · 2020-11-11T17:58:38Z

Addressed some of the PR comments; rebased off of current master.

Fix Python linking errors on some platforms. This appears to be related to the GCC visibility of methods overridden in header files, and is fixed by moving the implementation to the corresponding cpp files. Addresses the Travis CI failure of #1225

* Use pybind11/operators.h instead of explicit __eq__ method.

halfflat requested review from bcumming and noraabiakar November 9, 2020 22:13

noraabiakar suggested changes Nov 10, 2020

View reviewed changes

halfflat commented Nov 10, 2020

View reviewed changes

doc/python/simulation.rst Outdated Show resolved Hide resolved

halfflat commented Nov 10, 2020

View reviewed changes

python/simulation.cpp Outdated Show resolved Hide resolved

halfflat commented Nov 10, 2020

View reviewed changes

python/test/unit/test_simulator.py Show resolved Hide resolved

noraabiakar reviewed Nov 11, 2020

View reviewed changes

arbor/CMakeLists.txt Outdated Show resolved Hide resolved

bors bot added a commit that referenced this pull request Nov 11, 2020

Try #1225:

c4bb58c

bcumming requested changes Nov 11, 2020

View reviewed changes

halfflat added 5 commits November 11, 2020 18:16

Minor fixes

39237bf

Revert PIC property addition to arbor/CMakeLists.txt

2681d57

Address PR review.

14e1d9e

* Add comment re: PYBIND11_NUMPY_DTYPE. * Remove debug prints from unit_distributed/test_simulator.py. * Add `ci=None` to seaborn plot in single_cell_recipe.py. * Rename: `py_simulation` -> `simulation_shim`; `py_spike_recording` → `spike_recording`.

halfflat force-pushed the feature/python-probe-diversity branch from a630474 to 14e1d9e Compare November 11, 2020 17:58

noraabiakar mentioned this pull request Nov 12, 2020

Move inline overrides of public methods out of header files #1231

Merged

Address PR comments.

2516ef6

* Use pybind11/operators.h instead of explicit __eq__ method.

bcumming approved these changes Nov 17, 2020

View reviewed changes

noraabiakar approved these changes Nov 18, 2020

View reviewed changes

halfflat merged commit 1bfe96b into arbor-sim:master Nov 18, 2020

This was referenced Nov 20, 2020

C++ support for LFPy methods. #1115

Closed

Prepare extracellular potential demo for LFPy (Python) #1036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose diverse probes in Python API. #1225

Expose diverse probes in Python API. #1225

halfflat commented Nov 9, 2020 •

edited

Loading

thorstenhater commented Nov 10, 2020

noraabiakar left a comment

halfflat commented Nov 10, 2020

bcumming commented Nov 11, 2020

bors bot commented Nov 11, 2020

bcumming left a comment

bcumming Nov 11, 2020

halfflat Nov 11, 2020

bcumming Nov 17, 2020

halfflat Nov 17, 2020

bcumming Nov 11, 2020

halfflat Nov 11, 2020

bcumming Nov 17, 2020

bcumming Nov 11, 2020

halfflat Nov 11, 2020

bcumming Nov 11, 2020

halfflat Nov 11, 2020

halfflat commented Nov 11, 2020

		@@ -377,6 +377,8 @@ Overriding properties locally
		the morphology.


		.. _cable-cell-probes:

Expose diverse probes in Python API. #1225

Expose diverse probes in Python API. #1225

Conversation

halfflat commented Nov 9, 2020 • edited Loading

thorstenhater commented Nov 10, 2020

noraabiakar left a comment

Choose a reason for hiding this comment

halfflat commented Nov 10, 2020

bcumming commented Nov 11, 2020

bors bot commented Nov 11, 2020

try

bcumming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfflat commented Nov 11, 2020

halfflat commented Nov 9, 2020 •

edited

Loading