Remove NodeCollection pointer from Node class

[med-ayssar]

The Node class does not need to know about the primitive NodeCollection object it belongs to. Therefore, the NodeManange must keep track of the mapping between a Node instance and the NodeCollection container.

This is a derived requirement for the Vectorization project of the Node class.

[heplesser]

I am seriously concerned about the performance consequences of this change. For spatial connectivity, neurons need to know which node collection they belong to. I expect that this change will make lookup noticeably slower, even though std::lower_bound() has logarithmic complexity. We need to perform systematic benchmarks with spatial models before we proceed to avoid performance regressions on building.

I am also not happy with the mixed-camel-casing and would suggest clear_node_collection_container and similar.

[med-ayssar]

I am seriously concerned about the performance consequences of this change. For spatial connectivity, neurons need to know which node collection they belong to. I expect that this change will make lookup noticeably slower, even though std::lower_bound() has logarithmic complexity. We need to perform systematic benchmarks with spatial models before we proceed to avoid performance regressions on building.

I am also not happy with the mixed-camel-casing and would suggest clear_node_collection_container and similar.

The functions using the get_nc in the spatial.cpp have as parameter an array of IDs (i.e, an ArrayDatum of IDs). In order to avoid the possible runtime complexity that I have introduced, we Can group the items in the ArrayDatum by the NodeCollection they belong to, without explicitly searching for the NodeCollection, and then from each group we extract a representative of the group and call the new implemented get_nc function in the NodeManager on the representative item.

[med-ayssar]

@heplesser, I think last time you said that you might have been overthinking about the performance impact, but those new changes won't have any significant impact on the current performance.

Can we merge this PR, as I need it for the new devices?

[heplesser]

@med-ayssar Please see my comments inline. Also, the compiler seems to miss some things ;). My main concern is still performance. I will see if I can find some good benchmarks.

[jougs]

Happy if you merge all my suggestions.

[jougs]

@heplesser: can you please re-review? Thanks!

[heplesser]

@jougs Sorry for the delay. I definitely want to run some benchmarks to check performance effects. I have located suitable benchmarks and will run them within two weeks.

[med-ayssar]

@heplesser: I think it can be merged now :D

[heplesser]

Looks good now. I performed benchmarks small and large and did not find any noticeable slowdown.

[jougs]

I totally realize that it is bad style if

1. one of the original reviewers (me)
2. comments on a merged pull request
3. by a person not working on NEST anymore

🙈

But: couldn't this have been done like this?

Suggested change

	inline size_t
	NodeCollection::get_first() const
	{
	return ( *begin() ).node_id;
	}
	inline size_t
	NodeCollection::get_last() const
	{
	size_t offset = size() - 1;
	return ( *( begin() + offset ) ).node_id;
	}
	inline size_t
	NodeCollection::get_first() const
	{
	return begin()->node_id;
	}
	inline size_t
	NodeCollection::get_last() const
	{
	return ( end() - 1 )->node_id;
	}

Just sayin'...

[med-ayssar]

That was not that simple or better saying, things might get more ugly.

The begin function returns an iterator, and this iterator is a bit complex, as it hides the distinction between the primitive and composite types. Thus, begin()->node_id won't work and also the operator-() is not implemented.