Allow setting volume_transmitter via NodeCollection, instead of naked ID

[jougs]

This PR mainly fixes #2152 by allowing to set the volume_transmitter on the stdp_dopamine_synapse using a NodeCollection instead of a naked ID.

• As this breaks all existing code involving these two models, I took the liberty to also rename the property vt of stdp_dopamine_synapse to volume_transmitter as suggested in a comment in the original issue.

• As the weight_recorder is meant to be handled in pretty much the same way as the volume_transmitter, I also streamlined the code doing that, so it is more similar. I have also added a type check to that code, so it is less prone to errors.

• On the way, I added NodeCollection::create(Node*), which allows to conveniently create a NodeCollection from an existing node from a pointer.

• I also removed the apparently unused function CommonSynapseProperties::get_node()

[JanVogelsang]

We should probably add a 4.0-tag here, unless that's what "Behavior changes" stands for. But for me that sounds more like a "no API change, just behavioral changes" and that's not the case here.
Or do we not count renaming a parameter ("vt"->"volume_transmitter") as API change?

[jougs]

We should probably add a 4.0-tag here, unless that's what "Behavior changes" stands for. But for me that sounds more like a "no API change, just behavioral changes" and that's not the case here. Or do we not count renaming a parameter ("vt"->"volume_transmitter") as API change?

As we don't have too many releases and the major releases are often many years apart, we don't adhere to a no-API-changes-in-minor-versions policy too strictly. We usually try to deprecate things at least one release in advance, but have not been totally consistent with that if we deemed that only a small fraction of users will be affected by the change. For example, in NEST 3.4 we have removed some deprecated exotic models and changed the default port for NEST Server. In case of problems, we can always assist people on the mailing list or here in the issue tracker.

[YounesBouhadjar]

The PR looks good, we just need to correct #2168

[JanVogelsang]

Interestingly, clang can't compile the same code that gcc compiles.

[jougs]

Interestingly, clang can't compile the same code that gcc compiles.

Yes, this is odd. But then again, the code is also not really nice and I even struggled a bit to get it to compile on gcc...

In the new code we now have weight_recorder* CommonSynapseProperties::get_weight_recorder(). Then we have Event::set_receiver(Node&), which we want to use in Connector<ConnectionT>::send_weight_event().

My naive thinking was that I should be able to upcast the weight_recorder* obtained from the getter to Node* using a static_cast and dereference the result of that, all in a single line right in the call to set_receiver() like this:

wr_e.set_receiver( *static_cast< Node* >( cp.get_weight_recorder() ) );

This, however did not even work on gcc and I resorted to the dynamic_cast, which clang now seems unhappy about.

Interestingly, the equivalent C-cast seems to work (at least on gcc):

wr_e.set_receiver( *( Node* )( cp.get_weight_recorder() ) );

As does a two line construct that skips explicit casts altogether:

Node* wr_tmp = cp.get_weight_recorder();
wr_e.set_receiver( *wr_tmp );

I am leaning towards the last solution, as that is most explicit. What do you think, @JanVogelsang?

[JanVogelsang]

I don't understand why a simple static_cast wouldn't work here. Do you know the error it produced? The last solution isn't too bad, it's longer than it has to be and I don't feel like we have to be explicit in this place as no one really cares if the weight recorder is casted to a node-pointer before or not. But it's definitely a solution that one could use here which still is quite clean, so in the interest of saving time I think it might be the best solution right now.

[jougs]

Ha!

It definitely helps to read the full error message, including the notes 😅

/home/jochen/work/src/nest-simulator/models/nestkernel/connector_base_impl.h:58:25: error: invalid ‘static_cast’ from type ‘nest::weight_recorder*’ to type ‘nest::Node*’
   58 |     wr_e.set_receiver( *static_cast< Node* >( cp.get_weight_recorder() ) );
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/jochen/work/src/nest-simulator/models/nestkernel/connection.h:27:
/home/jochen/work/src/nest-simulator/models/nestkernel/common_synapse_properties.h:40:7: note: class type ‘nest::weight_recorder’ is incomplete
   40 | class weight_recorder;
      |       ^~~~~~~~~~~~~~~

The problem was that the file in question did not include weight_recorder.h and thus the type was not known and the cast could not be performed.

Fixed in db99d60.