Skip to content

Conversation

@JanVogelsang
Copy link
Contributor

When using Boost's integer_sort method, we were currently sorting the Sources by converting them to ints, which on most platforms are represented using 32 bits. For global node ids requiring more than 32 bits (>2,147,483,647), the sorting fails.

@JanVogelsang JanVogelsang added the T: Bug Wrong statements in the code or documentation label Dec 10, 2025
@JanVogelsang JanVogelsang self-assigned this Dec 10, 2025
@github-project-automation github-project-automation bot moved this to In progress in Kernel Dec 10, 2025
@heplesser heplesser added S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Dec 11, 2025

template < typename T >
inline int
inline uint64_t
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose uint64_t? Given that get_node_id() returns size_t, wouldn't that be a better choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_node_id() actually returns a uint64_t that's why I chose this type: https://github.com/nest/nest-simulator/blob/master/nestkernel/source.h#L56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we might just change the type used in Source used to store the node-id to size_t. It's identical to uint64_t on most platforms anyway and there is also no apparent reason why we would need exactly 64 bits there. We just need the same number of bits we use anywhere else for node-ids, which as you already stated is size_t.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that I had looked at the wrong get_node_id(), namely the one in SourceTable:

size_t
nest::SourceTable::get_node_id( const size_t tid, const synindex syn_id, const size_t lcid ) const
{
if ( not kernel().connection_manager.get_keep_source_table() )
{
throw KernelException( "Cannot use SourceTable::get_node_id when get_keep_source_table is false" );
}
return sources_[ tid ][ syn_id ][ lcid ].get_node_id();
}

It actually takes the value from Source and returns it. I think the best way to sort this out is indeed to change to size_t in Source.

@heplesser heplesser requested review from gtrensch and med-ayssar and removed request for gtrensch December 11, 2025 10:20
@med-ayssar
Copy link
Contributor

med-ayssar commented Dec 11, 2025

Is it not better to use decltype to deduce the return type?

decltype(boost::get<0>(s).get_node_id()) so basically you don't care about the precision type returned by the function, it will be always correct.

@heplesser
Copy link
Contributor

Is it not better to use decltype to deduce the return type?

decltype(boost::get<0>(s).get_node_id()) so basically you don't care about the precision type returned by the function, it will be always correct.

I hade been wondering if that was possible, but I also sometimes feel that with these new C++ constructs, one can make code more difficult to read. A plain size_t is much easier to parse and interpret that a 40 characted decltype(...).

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) S: High Should be handled next T: Bug Wrong statements in the code or documentation

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants