Problem with datahandle

[nrnhines]

8.2.2 works with following but not master. Latter generates

Assertion failed: file ../src/ivoc/graph.cpp, line 2486
NEURON: !pd_handle.refers_to_a_modern_data_structure()
 near line 0
 {hoc_pointer_(&_pysec.soma.v( 0.5 ))}
                                      ^
        doNotify()
Exception in gui thread

This is a general problem with Python or HOC sections. A simple way to reproduce is

1. Launch nrngui
2. Create a section. e.g. NEURONMainMenu/Build/singlecompartment
3. Pop up a Graph window. ie. NEURONMainMenu/Graph/Voltageaxis
4. In the Graph window menu select "Plot what"
5. In the symbol chooser that pops up, select "soma."
6. Then select "v(0.5)"
7. Pressing "Accept" will generate

Assertion failed: file ../src/ivoc/graph.cpp, line 2486
/home/hines/neuron/nrn/build/install/bin/nrniv: !pd_handle.refers_to_a_modern_data_structure()
 near line 0
 {hoc_pointer_(&soma.v( 0.5 ))}
                               ^
hoc_run1: caught exception: hoc_execerror: !pd_handle.refers_to_a_modern_data_structure()

[nrnhines]

A large proportion of the issue (> 98%) can be worked around with

$ git diff
diff --git a/src/ivoc/graph.cpp b/src/ivoc/graph.cpp
index e50a9ed00..0e3a3ed22 100644
--- a/src/ivoc/graph.cpp
+++ b/src/ivoc/graph.cpp
@@ -2483,14 +2483,17 @@ void Graph::choose_sym() {
         char buf[256];
         double* pd = sc_->selected_var();
         neuron::container::data_handle<double> pd_handle{pd};
-        assert(!pd_handle.refers_to_a_modern_data_structure());  // pd + i below would be
-                                                                 // problematic
         if (sc_->selected_vector_count()) {
             Sprintf(buf, "%s", sc_->selected()->string());
             GraphVector* gv = new GraphVector(buf);
             gv->color(color());
             gv->brush(brush());
             int n = sc_->selected_vector_count();
+            if (n > 1) {
+                assert(!pd_handle.refers_to_a_modern_data_structure());
+                // pd + i below is problematic
+            }
+
             for (int i = 0; i < n; ++i) {
                 gv->add(double(i),

That is, all the cases where pd points to a scalar. However the issue continues to exist when the symbol data is an array.
E.g. try
neurondemo,
select the Release demo,
pop up a Graph with NEURONMainMenu/Graph/VectorMovie,
select the Graph menu item Plot what,
navigate to and select the ca_cadifpmp[all](0.5) item,
and select Accept.

In this case, commenting out the assert suffices for correct plotting.

@1uc @olupton In a more fundamental sense, I don't understand why pd_handle is not supposed to refer to a modern data handle given that there is provision in soa for range variable arrays (SoAoS). Is a proper fix a rewrite of the internals of

double* pd = sc_->selected_var();

so that it returns a data_handle instead of a double*? Presumably SymChooser.selected_var can analyse the string a bit more with respect to its array characteristics.

[1uc]

I don't know the answer, but what you're suggesting makes sense. I'll make time tomorrow to look into this.

One comment: when reading through the SoA, I saw that it has the ability to "upgrade" a raw pointer to a proper data_handle. One ctor will attempt to upgrade the raw pointer:

auto dh = data_handle(ptr);

while the other doesn't:

auto dh = data_handle(do_not_search_t, ptr);

See (respectively):

nrn/src/neuron/container/data_handle.hpp

Line 74 in 5ea534f

auto needle = utils::find_data_handle(raw_ptr);

nrn/src/neuron/container/data_handle.hpp

Line 91 in 5ea534f

data_handle(do_not_search_t, T* raw_ptr)

[1uc]

Here's what's strange: The code expects a raw-pointer (wrapped as a data_handle); and fails when given a proper "modern" data_handle. However, the "modern" case aught to be the simple case. Hence, I suspect this might just be a case of not fully modernizing that piece of code and leaving us with an assert, so we can find when the issue manifests.

I found the following method:

nrn/src/neuron/container/data_handle.hpp

Lines 94 to 116 in 5ea534f

	/**
	* @brief Get a data handle to a different element of the same array variable.
	*
	* Given an array variable a[N], this method allows a handle to a[i] to yield a handle to a[j]
	* within the same logical row. If the handle is wrapping a raw pointer T*, the shift is applied
	* to that raw pointer.
	*/
	[[nodiscard]] data_handle next_array_element(int shift = 1) const {
	if (refers_to_a_modern_data_structure()) {
	int const new_array_index{m_array_index + shift};
	if (new_array_index < 0 || new_array_index >= m_array_dim) {
	std::ostringstream oss;
	oss << *this << " next_array_element(" << shift << "): out of range";
	throw std::runtime_error(oss.str());
	}
	return {m_offset,
	static_cast<T* const*>(m_container_or_raw_ptr),
	m_array_dim,
	new_array_index};
	} else {
	return {do_not_search, static_cast<T*>(m_container_or_raw_ptr) + shift};
	}
	}

which would suggest we remove the assert completely and update pd + i to

   gv->add(double(i), pd.next_pd.next_array_element(i));

I've created a PR (#2463) with the proposed change. Locally it passed the reproduction steps outlined above. (By virtue of removing the assert.)

For raw pointers I think this changes nothing. For "modern" data_handles I can't be sure this is the correct thing. The method next_array_element will iterate over the "array_dims" one "SoA field". Put differently, the next_array_element requires that if n > 1 and the handle is "modern", then the underlying storage is a "SoAoS" field and has array_dim == n.

[nrnhines]

I was writing this when your previous comment arrived. This now is more or less obsolete, but I think it is still worth adding to the discussion.

It was clear, but it's becoming even more clear, that one cannot satisfyingly develop NEURON without a fairly deep clarity about everything SoA and data handle in nrn/src/neuron. That seems to be the case here. I.e. the code DOES upgrade the pointer to a modern data handle via neuron::container::data_handle<double> pd_handle{pd}; (Or is that NOT equivalent to auto pd_handle = data_handle(ptr);?) It is just that my level 1 clarity about this is in the domain of scalar RANGE variables. And I'm very uncertain about array RANGE variables, Vectors, and HOC user declared scalars and double arrays.

A similar situation has arisen with my work on #2460. In this case, the mod file holds a static pointer to a Prop and it would seem more correct to upgrade it to a data handle for safety against permutation and invalidation.

[1uc]

(Or is that NOT equivalent to auto pd_handle = data_handle(ptr);?)

They look like they're the same thing.

Your comment is useful, because it tells me things we need to check. I think we need to check how:

1. "array RANGE" variables,
2. Vectors,
3. and user declared scalars and double arrays
   are stored. Is it in SoA containers with what type of field; or is is just malloc?

Regarding the logic of the loop:

            for (int i = 0; i < n; ++i) {
                gv->add(double(i), data_handle(pd + i));
            }

what is this loop over? When we're dealing with a scalar RANGE variable is n == 1? When dealing with an array RANGE variable is n > 1? (All three questions are somewhat equivalent.)

[nrnhines]

gv->add(double(i), pd.next_pd.next_array_element(i));

I agree. That seems like the correct fix to this problem. Or at least one step closer to being a complete solution. If it works for all RANGE, and Vector, I think we can declare success for this issue. I'll do some (I think it has to be manual) testing.

[nrnhines]

what is this loop over?

The SymbolChooser has a special string for array variables. One can see an example with
E.g. try

neurondemo,
select the Release demo,
pop up a Graph with NEURONMainMenu/Graph/VectorMovie,
select the Graph menu item Plot what,
navigate to and select the ca_cadifpmp[all](0.5) item,
and select Accept.

In this case the array dimension comes from https://github.com/neuronsimulator/nrn/blob/master/share/demo/release/cabpump.mod in the lines

DEFINE NANN  10

and

	ca[NANN]	(mM) <1e-6> : ca[0] is equivalent to cai

And if you look at a Plot what? and select a Vector instance, there is a corresponding x[all].
I have not looked to see if there is an equivalent for HOC double foo[5].

[1uc]

Thank you. I think now I understand.

[1uc]

I'll close because this should have been fixed by #2463.