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

Fixing bad coding pattern in loops in connection creation #2597

Merged
merged 9 commits into from
Mar 10, 2023

Conversation

JanVogelsang
Copy link
Contributor

We cast to int in a loop so that we can do --i to counteract the loops ++i if a connection is not created. Instead, target_number_connections should just be counted down in a while loop here.

@JanVogelsang JanVogelsang added S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) need reviewer Ready to be reviewed but no reviewer assigned labels Jan 30, 2023
@JanVogelsang JanVogelsang self-assigned this Jan 30, 2023
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

This is indeed much nicer. @heplesser: can you please also review?

nestkernel/connection_creator_impl.h Outdated Show resolved Hide resolved
nestkernel/connection_creator_impl.h Outdated Show resolved Hide resolved
nestkernel/connection_creator_impl.h Outdated Show resolved Hide resolved
nestkernel/connection_creator_impl.h Outdated Show resolved Hide resolved
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

There's another one in lines 737-750. Please also change that.

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.

This looks good. Could your add const for local variables in the loops that lack it but could be const, e.g., random_id or source_id.

@jougs
Copy link
Contributor

jougs commented Feb 20, 2023

@JanVogelsang: friendly ping!

@JanVogelsang
Copy link
Contributor Author

Cancelled after 360min, how could that happen?

@jougs
Copy link
Contributor

jougs commented Feb 23, 2023

I have no clue how this could take so long. I have restarted the jobs (again, after they timed out again after my first restart attempt). Let's see what happens.

@gtrensch gtrensch added this to PRs in progress in Kernel via automation Feb 24, 2023
@heplesser
Copy link
Contributor

The problem causing the timeout is here:

// We bail out for non-local neurons only now after all possible
// random numbers haven been drawn. Bailing out any earlier may lead
// to desynchronized global rngs.
if ( not kernel().node_manager.is_local_node_id( target_id ) )
{
continue;
}

Before the continue, we need to insert --target_number_connections to account for connections created on other VPs (this is fixed_outdegree which does not parallelize: all VPs need to draw all connections and then just skip creating those they are not responsible for).

@JanVogelsang
Copy link
Contributor Author

Thanks Hans Ekkehard, I'm mad at me to not notice this myself earlier. I fixed it, let's see if the testsuite runs through now.

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.

@JanVogelsang Thanks! Just a small suggestion for a better comment so we won't wonder a year from now why we decrease the counter half-way through the loop.

nestkernel/connection_creator_impl.h Outdated Show resolved Hide resolved
Kernel automation moved this from PRs in progress to PRs pending approval Mar 10, 2023
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
@heplesser heplesser merged commit 50fe5c0 into nest:master Mar 10, 2023
Kernel automation moved this from PRs pending approval to Done (PRs and issues) Mar 10, 2023
@terhorstd terhorstd added the T: Maintenance Work to keep up the quality of the code and documentation. label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) need reviewer Ready to be reviewed but no reviewer assigned S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants