probe id -> probeset id, and clarification in docs

[brenthuisman]

• Rename probe id and probe id to probeset id and probeset_id to clarify that they are associated to a (loc)set.
• Clarify documentation.

Fixes #1877, #1484

[Helveg]

I didn't go over the code changes, just the RST. I skipped the 2 cpp docs because they made my head spin. Unless this level of terseness makes cpp devs happy, Id' opt for a full rewrite of cpp/probe_sample.rst, there's so much technicalities and abstract justifications of the design mixed into the text that I have a hard time finding information in it. I'd start by introducing the concepts, with simple examples, and then layering in the more detailed things towards the end of the doc.

I believe probeset address should be replaced by probeset. Is it still required to include the term address for anything?

[Helveg]

So do we want to call them probesets or probeset addresses. I thought we'd go for just probesets.

The explanation here seems to contort itself a bit around the unique case of a probeset being described by a 1-loc locset. Shouldn't the explanation just always talk about probesets as multiple probes, the user can assume that 1-loc locsets and 0-loc locsets would produce 1 and 0 probes in the probeset?

[thorstenhater]

Technically it might be worse: Locsets are what's usually called a multiset, ie a set of unique keys
(called the support of the multiset) and an associated multiplicity for each key. Probes should be
defined on the support, thus each location is probed only once. So, the number of points in the locset
might be larger than the number of unique points.

[brenthuisman]

sim.samples(handle) will return a list of length K. In each element, you'll find a tuple containing metadata and data, where metadata will be a string describing the location, and data a numpy.ndarray. The structure of the numpy array can vary (and may not be a numpy array), that's at the bottom of the documentation for samples().

I reshuffled the text there a bit, hopefully it made this clearer.

[thorstenhater]

You know me ... a graphic? A crude approximation

O-------------         Cell morphology
^    ^    ^            Probe locset
|    |    |
L0 L1 L2               Locations, eg (cable 0 0.23)
U0 V0 W0               Values, one column per Location
U1                     one entry per sampling time point
U2
...

[thorstenhater]

Thanks for adding the graphic, but I think we could be even clearer by splitting it in two

1. Showing the relation of handle, probeset, probes, and their locations
2. Showing the relation between the sampled data and the probeset

Sorry for being picky here, but we inflict multiple layers of callbacks and handles on our
users (for good reasons), so giving them a hand is needed.

That said, I made one of the images, so maybe that plus the more schematic one you added
is enough. Tell me what you think.

Also, since I cannot comment on full files, feedback for the graphic here

1. Typo: experession
2. Did you check array format? I thought I remember columns of time, value. As shown it indicates rows. Maybe this would be clearer if shown as a table.
3. The -> relation is unclear

• handle -> probeset: obtain a probeset from simulation
• probeset -> probe, probe -> data|meta: ownership
• meta -> locset, data -> ndarray: is a

On 3. I would suggest showing 'has a' and 'is a' relations like this; example for probe

+---------------+
| Probe         |
| meta: locset  | 
| data: ndarray |
+---------------+

The handle -> probeset -> probe could remain.

[Helveg]

If we're doing :term:`probeset_id` then there's a lot of missed occurences that are just probeset_id

[Helveg]

I wouldn't say that probes are described by probeset addresses. Maybe "probesets are described by probeset addresses" but I really still don't see the distinction between the two, what's a probeset address? The cell_member? That is usually already called the probeset id.

[brenthuisman]

Thanks for the review! I've taken most into account.

The probeset address is a distinction only relevant for the C++ API, you won't need to know what they are when using Python. I agree the distinction makes it a bit muddy, but at least on the C++ side it needs to stay. I think I've eradicated every mention of probeset addresses from the Python pages, which I think must have started as a copy-paste of the C++ API. Please have a look, maybe there's still some stuff left that now doesn't make sense anymore.

The definitions were moved to the concept section (logical, right?) so the probeset address def does not confuse.

I left your comments on probeset address on the cpp pages open, because @thorstenhater will have a quick skim and it's good to have him confirm.

[brenthuisman]

sim.samples(handle) will return a list of length K. In each element, you'll find a tuple containing metadata and data, where metadata will be a string describing the location, and data a numpy.ndarray. The structure of the numpy array can vary (and may not be a numpy array), that's at the bottom of the documentation for samples().

I reshuffled the text there a bit, hopefully it made this clearer.

[Helveg]

Found some more sections where probe and probeset were used interchangeably. Other than that it's becoming clearer I think :) The new terms seem to work at conveying the message.

[Helveg]

Nice endeavour :)

[thorstenhater]

A few minor comments.

[thorstenhater]

A general observation on this part of the docs: A graph of the process could help.

[brenthuisman]

Which process is that exactly? Of creating your own probe?

[thorstenhater]

You know me ... a graphic? A crude approximation

O-------------         Cell morphology
^    ^    ^            Probe locset
|    |    |
L0 L1 L2               Locations, eg (cable 0 0.23)
U0 V0 W0               Values, one column per Location
U1                     one entry per sampling time point
U2
...

[brenthuisman]

Shortcut to the requested schematic: https://github.com/arbor-sim/arbor/pull/1898/files#diff-119b7780b97222f316ae7b9bf31a04ac8d477cc2c07f1dcbcd33722b14b23222

[Helveg]

Great image. The only thing that's omitted is the scalar vs vector valued probing

[thorstenhater]

Being picky on the graphics, sorry. I make amends as soon as I figure out how to upload an SVG here.

[thorstenhater]

Thanks for adding the graphic, but I think we could be even clearer by splitting it in two

1. Showing the relation of handle, probeset, probes, and their locations
2. Showing the relation between the sampled data and the probeset

Sorry for being picky here, but we inflict multiple layers of callbacks and handles on our
users (for good reasons), so giving them a hand is needed.

That said, I made one of the images, so maybe that plus the more schematic one you added
is enough. Tell me what you think.

Also, since I cannot comment on full files, feedback for the graphic here

1. Typo: experession
2. Did you check array format? I thought I remember columns of time, value. As shown it indicates rows. Maybe this would be clearer if shown as a table.
3. The -> relation is unclear

• handle -> probeset: obtain a probeset from simulation
• probeset -> probe, probe -> data|meta: ownership
• meta -> locset, data -> ndarray: is a

On 3. I would suggest showing 'has a' and 'is a' relations like this; example for probe

+---------------+
| Probe         |
| meta: locset  | 
| data: ndarray |
+---------------+

The handle -> probeset -> probe could remain.

[thorstenhater]

There!

[brenthuisman]

Great image. The only thing that's omitted is the scalar vs vector valued probing

What probe returns vector data? I don't think I actually ever saw one...

[brenthuisman]

I have tweaked the image, added an example of data and added words.
https://raw.githubusercontent.com/arbor-sim/arbor/356ecbcc2b77f7a65089ab74c94d6375e37d7ef2/doc/python/probe_sample-diag.svg

I prefer this image, because it shows what you probably care about: how to get to your sampling data, and why a handle is involved. This is now consistent, without going into a tangent about the precise kind of relationship. The probeset box is now clearly separate, and hopefully illustrates that a handle is associated to it, but gives access to its individual probes.

[Helveg]

Great image. The only thing that's omitted is the scalar vs vector valued probing

What probe returns vector data? I don't think I actually ever saw one...

Me neither, but the docs mentioned it, so I thought they exist.

[brenthuisman]

Strictly speaking, it's up to the (C++) probe to define what it returns, and also whether that is one or more values per timestamp (or a timestamp at all). Although it can be any structure, as far as I know the ones we have now return an ndarray, as shown in the example.

[thorstenhater]

In Python we do not have user-defined callbacks. Thus ndarray and its format is fixed, it'll always have
one column for time and each of the probe locations and one row per sampling point.

[thorstenhater]

Thanks for improving this. Over experimenting with the API and reading the code in
more depth for this review it became clear that there is still some potential
confusion.

For reference, I'll set up an example

class recipe(A.recipe):
    # ...
    
    def probes(self, _):
        if gid == 0:
            return [A.cable_probe_ion_diff_concentration_cell("na"),
                    A.cable_probe_membrane_voltage("(location 0 0)"), ]
        else:
            return [A.cable_probe_ion_diff_concentration_cell("ca"), 
                    A.cable_probe_membrane_voltage("(terminal)"), ]
        
    # ...

R = recipe()
S = A.simulation(R())

ts = A.regular_schedule(0.1)
hdls = [S.sample((0, 0), ts), # Sodium in gid 0 
        S.sample((0, 1), ts), # U_m in gid 0 soma
        S.sample((1, 0), ts), # Calcium in gid 1
        S.sample((1, 1), ts), # U_m in gid 1 terminals
]

sim.run(tfinal=0.5)

for hdl in hdls:
    for data, meta in S.samples(hdl):
       pass

Here we extract three vector probes and one scalar. For the sake of the
example assume two cells; cell 0 has N segments and cell 1 M segments of which T are
terminals. The schedule evaluates to five points [0, 0.1, 0.2, 0.3, 0.4]. We
were to tabulate the result we would see something like this

Handle	Meta	Data	Quantity	Vector
(0, 0)	[(cable ..), ...] length N	NDArray[N + 1, 5]	Nad	Y
(0, 1)	(cable ..)	NDArray[2, 5]	Um	N
(1, 0)	[(cable ..), ...] length M	NDArray[M + 1, 5]	Cad	Y
(1, 1)	(cable ..)	NDArray[2, 5]	Um	N

Regarding the improved figure, thanks for keeping up with it. In the light of the above
we should rework s.t.

• meta is a list of locations iff a vector probe is sampled else a single location
• data is a table with one column per location above plus one for the time
• why is a handle associated with a set? In my experiments never seemed to be that? Is there a fundamental difference Py/C++? If so we need a huge red warning banner
• what does the dashed backedge do?
• why does every probe have a connection from a single handle?

Feel free to use and adapt the above example.

Further it should be clarified

• _cell probes are vector probes, everything is a scalar
• how the locations for _cell probes are made
• handle associates has (cell, num of probe on cell)
• asking for a nonexistent probe (23, 42) will get something empty not an error
• even if you -- in Python at least -- probe on locsets with cardinality > 1 you'll get one location only (BUG?)
• thus it's more accurate cable_probe_XYZ(location) instead of locset
• no user callbacks in Python

[brenthuisman]

I had to refresh my memory a bit.

1. I can update the image and description to reflect the fact that, depending on the type of probe, a probe may result in vector data or point data. I suppose I could borrow your example and paste it in. I believe this addresses your first two questions (re meta, data).
2. Why a handle is associated with a set: I can't answer the existential question in here, but I can confirm it is so, to the extent I tested it, solely with Python. Looking at the code, I had no reason to believe this was different in C++. Could you share more details on your experience to the contrary?
3. The dashed backedge is meant to demonstrate the association of a handle to a probeset. I will agree it can be improved by changing the arrow into a bi-directional one. Would that satisfy you?
4. Every probe has a connection from a single handle: the shaded box around it indicates a list, but better would be to add that explicitly in the caption.

Can the further clarifications wait for a second PR (in the interest of not letting this one slide any further than it has)?

[thorstenhater]

I'd love to make it correct on the first possible moment, since we risk users coming up with misconceptions otherwise.
I am really confused by the issue that non-cell-type probes seem to be scalar, meaning that the intuitive

probe(locset('(terminal)'), membrane_voltage)

does not in fact return a list of voltages at all terminal points, but rather at one of those points.

Could you confirm this behaviour? Just tweaking the single_cell_recipe example should be sufficient.
If confirmed, we should potentially consider this a bug.

I can update the image and description to reflect the fact that, depending on the type of probe, a probe may result in vector data or point data. I suppose I could borrow your example and paste it in. I believe this addresses your first two questions (re meta, data).

The point here is that rather than returning a [(meta, values_over_time)] a vector probe gives (list_of_locations, values_for_locations_over_time). Difference being in the shape of the data.

[brenthuisman]

Let's start at simulation.sample() and go both ways.

1. simulation.sample() returns a sampler_association_handle, https://github.com/arbor-sim/arbor/blob/master/python/simulation.cpp#L137
2. sampler_association_handle is a integer number, https://github.com/arbor-sim/arbor/blob/master/arbor/include/arbor/sampling.hpp#L43
3. Conclusion: a handle is to 1 probe, located at 1 position. Not to a probeset! All probes get a cell local id, regardless of being grouped together by way of being constructed from 1 locset. I confirm your observation. TODO: modify the image and text to reflect that a handle refers to a single probe.

The other direction:
5. simulation.sample() is called with a arbor.cell_member (or tuple for short), which betrays the flatness of the return value. arbor.cell_member only specifies gid and index, and index is over a "cell-local collection", in this case, over probe_ids, NOT probeset_ids. TODO: update documentation.
6. sim.samples() returns a recorder, a new word to add to our glossary. https://github.com/arbor-sim/arbor/blob/master/python/simulation.cpp#L38
7. A recorder is a struct holding meta (can be anything but afaik always a log/reg expr, the number of records, and the records (sample_records), https://github.com/arbor-sim/arbor/blob/master/python/pyarb.hpp#L20
8. A sample_record holds a time and a util::any_ptr. Afaik they are right now always either scalar or vector values. https://github.com/arbor-sim/arbor/blob/master/arbor/include/arbor/sampling.hpp#L32 TODO: update rightmost column in image: the expression is for the loc a probe is attached to, NOT locset a probeset is attached to. Also, indicate value can be scalar or vector.

Now, the term probeset_id is unused. They don't have ids. probeset still might be used to describe a thing like arbor.cable_probe_membrane_voltage('"my_locset"'). However, these objects don't return anything interesting.usable, so other than that we might say they describe a probeset, it is not a very important term to define. However, that might change, pending #1929 (for which I volunteer). For now, TODO remove term probeset_id.

Questions:

1. When you say The point here is that rather than returning a [(meta, values_over_time)] a vector probe gives (list_of_locations, values_for_locations_over_time)., you mean that is incorrect (see probe(locset('(terminal)'), membrane_voltage) example), or that it is what you expect/like to see? Is this what you suggest we consider a bug? In the bit above I describe how things are now, but there is logic to expecting to have and be able to use a probeset_id. Is that what you're saying?
2. Nowhere are "scalar probes" and "vector probes" defined. TODO: define them, and define for each probe whether they are vector or scalar. Question: which are the vector ones? Those about mech state?

[thorstenhater]

There two levels to this:

1. For efficiency, the behviour in Python and C++ is substantially different
   • Python: Scalar probes give (by whatever mechanism) a record shaped like (this is not! the actual type) (location, array[time, values]) where time and values are 1d arrays with lengths corresponding to time elapsed/sampling freq.
     Vector probes get ([location], array[time, values]) where values now has a second dimension corresponding to locations length.
   • C++: We get callbacks, but essentially the same data layout.
2. Despite the intuitive idea of 'if probing a multi-location locset, we get a vector probe' we only get vectors when using the _cell kinded probes. This is what I might consider a bug or at least violating good UX.

We need to document clearly both these points and think whether we want to change 2.

[thorstenhater]

Another comment regarding #1929: Spikes are not recorded using the mechanism of probing.

[brenthuisman]

Thanks for clarifying that _cell kinded probes are those that output vector data.

So, after checking, the nonobvious difference between a probeset and a _cell kinded probes (or vector probe) is this:

1. The points in a probeset translate to items (corresponding to each point) in the list returned by .samples(). (contrary to earlier claims!)

   1. e.g.: a voltage probe placed on "(join (location 0 0.5) (location 0 0.7))" might return [(array([[ 0., -40.]]), (location 0 0.7)), (array([[ 0., -40.]]), (location 0 0.5))]. The handle points at a single index. So, the term probeset does make sense and should be left in.

2. The 'points' in a vector probe translate to columns in the data of the first and only element in the list returned by .samples().

   1. e.g.: sample [(array([[ 0., -40., -40., -40.]]), [(cable 0 0 0.333333), (cable 0 0.333333 0.666667), (cable 0 0.666667 1)])]

We can call it a bug. I've update #1929 to correct this.

[brenthuisman]

Updated, see https://arbor--1898.org.readthedocs.build/en/1898/python/probe_sample.html for the new diagram. I also added a definition for vector probes, and marked them if they are.

I think it's now clear what the difference is, and #1929 will remove that difference, as soon as I get back. Please do merge in the meantime :)

[Helveg]

Last duplicated concept: probe (arbor.probe) and probe (element of probeset)

[thorstenhater]

Here's a script to demonstrate the difference.

import arbor

tree = arbor.segment_tree()
p = tree.append(arbor.mnpos, arbor.mpoint(-3, 0, 0, 3), arbor.mpoint(3, 0, 0, 3), tag=1)
tree.append(p, arbor.mpoint(3, 0, 0, 3), arbor.mpoint(-3, 0, 0, 3), tag=2)
tree.append(p, arbor.mpoint(3, 0, 0, 3), arbor.mpoint(-3, 0, 0, 3), tag=2)

decor = (
    arbor.decor()
    .set_property(Vm=-40)
    .paint('"soma"', arbor.density("hh"))
    .place('"midpoint"', arbor.iclamp(10, 2, 0.8), "iclamp"))

cell = arbor.cable_cell(tree, labels, decor)

class single_recipe(arbor.recipe):
    def __init__(self):
        arbor.recipe.__init__(self)

    def num_cells(self):
        return 1

    def cell_kind(self, gid):
        return arbor.cell_kind.cable

    def cell_description(self, gid):
        return cell

    def probes(self, gid):
        return [arbor.cable_probe_membrane_voltage('(location 0 0.5)'),
                arbor.cable_probe_membrane_voltage_cell(),
                arbor.cable_probe_membrane_voltage('(join (location 0 0) (location 0 1))'),
                ]

    # (4.6) Override the global_properties method
    def global_properties(self, kind):
        return arbor.neuron_cable_properties()

recipe = single_recipe()
sim = arbor.simulation(recipe)
handles = [sim.sample((0, n), arbor.regular_schedule(0.1))
           for n in range(3) ]
sim.run(tfinal=1)

for hd in handles:
    print("Handle", hd)
    for d, m in sim.samples(hd):
        print(" * Meta:", m)
        print(" * Payload:", d.shape)

Handle 0
 * Meta: (location 0 0.5)
 * Payload: (10, 2)
Handle 1
 * Meta: [(cable 0 0 1), (cable 0 1 1), (cable 1 0 0), (cable 2 0 0), (cable 1 0 1), (cable 2 0 1)]
 * Payload: (10, 7)
Handle 2
 * Meta: (location 0 0)
 * Payload: (10, 2)
 * Meta: (location 0 1)
 * Payload: (10, 2)
"""
Thus
- 0: Single scalar probe.
- 1: Single vector probe.
- 2: Two scalar probes.

Conclusion: no bug present.

Might be worth to include this minimal example in the tutorials and/or discussion of probes.

[brenthuisman]

Thanks @thorstenhater, that's almost the script I have here :) I'll add it as an example, but I'll remove it at soon as the shape of record outputs is made consistent between probeset and vector probes.

[thorstenhater]

👍