Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make thread id available inside connection's constructor and set_status #2921

Closed
wants to merge 3 commits into from

Conversation

JanVogelsang
Copy link
Contributor

This PR solves #2902. Some model parameters could potentially depend on the RNG (see NESTML), while each generator is bound to a specific thread. Currently, connections (i.e., synapses) are not aware of the thread that they correspond to. One solution would be to add the thread id to the connection class. However, while this wouldn't increase the total object size in the case of index-based node targeting (HPC case, TargetIdentifierIndex) due to a 16-bit padding that is currently unused, it would possibly increase the size when using the pointer-based node targeting (non-HPC case, TargetIdentifierPtrRport). In theory, it could be possible to restructure the connection class members in order to include the thread id in both cases without increasing the total object size, however this would not be guaranteed for all systems/architectures.

Another solution would be to simply pass the thread id as parameter to all relevant functions of the connection class. When calling a function on a connection, it will always happen inside a threaded environment, therefore the thread id will always be available at that time. The only exception are default connections, which are not bound to a specific thread. In these cases however, the RNG of the main thread (thread id 0) can be used, if required at all.

@JanVogelsang JanVogelsang added T: Enhancement New functionality, model or documentation S: High Should be handled next I: External API Developers of extensions or other language bindings may need to adapt their code I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know labels Sep 6, 2023
@JanVogelsang JanVogelsang self-assigned this Sep 6, 2023
@JanVogelsang
Copy link
Contributor Author

@heplesser Could you review this or alternatively choose another reviewer?

@JanVogelsang JanVogelsang marked this pull request as draft September 6, 2023 09:33
@JanVogelsang JanVogelsang marked this pull request as ready for review September 6, 2023 11:35
@heplesser heplesser requested a review from suku248 September 7, 2023 06:49
@heplesser
Copy link
Contributor

Adding @suku248 as a reviewer as this touches upon connection representation (even though it does not add data to connections in this PR).

@pnbabu Could you provide a use case that needs the thread_id for the synapse?

@pnbabu
Copy link
Contributor

pnbabu commented Sep 7, 2023

@heplesser the functions random_normal() and random_uniform() in NESTML when translated to NEST code require a random number generator dependent on the thread ID. This thread ID is available in the Node class with the private get_thread() function, however, the Connection class does not have one (except for the send() function).

For example, if the NESTML model contains:

synapse stdp_stp: 
   ...
   parameters:
        U real = random_normal(0.5, 0.25)

The equivalent NEST code would be:

template < typename targetidentifierT >
stdp_stp__with_lif_sorn< targetidentifierT >::stdp_stp__with_lif_sorn() : ConnectionBase()
{ 
  P_.U = ((0.5) + (0.25) * nest::normal_distribution( nest::get_vp_specific_rng( get_thread() ) )); // as real
  ...
}

The above code results in a compilation error for get_thread() not being defined. In this particular situation, the thread ID is required in the constructor. However, because random_ functions can potentially be used in any block or a user-defined function within the synapse model, it would be beneficial to have the thread ID within the Connection class.

Adding @jougs as a reviewer since we had discussions regarding this.

@pnbabu pnbabu requested a review from jougs September 7, 2023 09:47
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the NEST Kernel Team meeting on Thursday, passing the thread around should not be necessary since in almost all cases the synaptic parameters will be drawn by the thread responsible for the target neuron, so just getting the current thread will give the necessary information.

The next step will be to check this. If this holds, code could be much simplified.

@heplesser heplesser removed the request for review from suku248 September 10, 2023 15:54
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Nov 10, 2023
@heplesser heplesser requested review from heplesser and removed request for jougs November 16, 2023 14:16
@heplesser
Copy link
Contributor

@JanVogelsang @pnbabu As I pointed out earlier, I think we can just use kernel().vp_manager.get_thread_id() whenever a random number would be required during connection creation, because the connection always belongs to the target thread and is created by its thread. Then, we do not need to pass or store the thread id.

Do you think this is a viable idea? @pnbabu Could you test this with code generated from NESTML?

@JanVogelsang
Copy link
Contributor Author

Yes, that sounds like a good idea.

@github-actions github-actions bot removed the stale Automatic marker for inactivity, please have another look here label Nov 18, 2023
@heplesser
Copy link
Contributor

I will close this PR now in favour of a different solution as discussed above.

@heplesser heplesser closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: External API Developers of extensions or other language bindings may need to adapt their code I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants