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

Correct errors in slicing of node collections #3132

Merged
merged 59 commits into from
May 26, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
738411e
Add tests detecting slicing errors
heplesser Mar 3, 2024
1162ca0
Remove accidentally commited files
heplesser Mar 3, 2024
99cc9ae
Move nest_types.h to libnestutil to allow inclusion in numerics.h
heplesser Mar 4, 2024
51ce783
Add algorithms for modular invers and first index of doubly periodic set
heplesser Mar 4, 2024
727c780
Fix errors in NodeCollection::*local_begin() implementations and rena…
heplesser Mar 4, 2024
33fa658
Convert FixedIndegree ConnBuilder to new iteration scheme
heplesser Mar 4, 2024
ac566d2
Merge branch 'master' of github.com:nest/nest-simulator into fix_nc_s…
heplesser Mar 4, 2024
b7f9627
Skip thread-dependent test if no threads
heplesser Mar 4, 2024
f5c54b7
Tidied filename
heplesser Mar 4, 2024
79f17c0
Replaced coordinates with linear integer coords for ease of debugging
heplesser Mar 4, 2024
a9bb077
Build position information as part of spatial metadata directly inste…
heplesser Mar 4, 2024
9cb1685
Revert "Convert FixedIndegree ConnBuilder to new iteration scheme"
heplesser Mar 6, 2024
6d56e62
Improved documentation and renamed some methods for clarity
heplesser Mar 7, 2024
4c5212b
Document algorithm for finding {thread,rank}_local_begin()
heplesser Mar 7, 2024
e0f0028
Fix incorrect variable name
heplesser Mar 7, 2024
ce0fdea
Merge branch 'master' of github.com:nest/nest-simulator into fix_nc_s…
heplesser Mar 14, 2024
d2071ff
Return 'positions' element of 'spatial' also if no local nodes.
heplesser Mar 14, 2024
386cae3
first_index() now returns size_t and terminates early if period == step
heplesser Mar 14, 2024
25e72a2
{rank,thread}_local_begin now work correctly if first_index returns i…
heplesser Mar 14, 2024
eef1a83
Test for #3108 now run serially and for 3 and 4 MPI procs
heplesser Mar 14, 2024
d518288
Improve documentation and variable naming, handling of a≥m case, and …
heplesser Mar 15, 2024
a09b218
Simplify code for finding first index exploiting mathematical simplif…
heplesser Mar 18, 2024
2d9d98f
Add further tests to rank/thread_local_begin
heplesser Mar 19, 2024
79c7732
Add functions to help debugging {rank,thread}_local_begin and improve…
heplesser Mar 21, 2024
c7ddfff
Extend test set for node collection iterators
heplesser Mar 21, 2024
590ee52
Extended tests for slicing
heplesser Mar 25, 2024
34c24f5
Extended documentation on NC slicing, started to implement new iterat…
heplesser Mar 25, 2024
3efe6c9
Add one more test case
heplesser Apr 11, 2024
33d6886
Fix cva trie
heplesser Apr 11, 2024
0aafccd
Straighten out node collection iteration, still not quite right
heplesser Apr 11, 2024
b4310df
Further work towards slicing
heplesser Apr 12, 2024
2392474
Further work on slicing, not fully working yet
heplesser Apr 15, 2024
6449f76
Some more progess
heplesser Apr 16, 2024
c7c0a92
Fixed some errors in tests
heplesser Apr 17, 2024
aaf7131
Add case distinction between GLOBAL and *_LOCAL stepping
heplesser Apr 17, 2024
0d09cd3
Slicing now works for all test cases; some revision of documentation …
heplesser Apr 17, 2024
6fc3b05
Fix corner case in rank/thread local begin
heplesser Apr 17, 2024
c89161a
Fix sli-related issue
heplesser Apr 17, 2024
ab7b267
Add some more logging
heplesser Apr 18, 2024
7003792
Add comment on TokenArray::push_back()
heplesser Apr 18, 2024
5953683
Changed cumul_abs_size/first_in_part to full size of parts
heplesser Apr 18, 2024
46195d6
Implement operator++() explicitly
heplesser Apr 18, 2024
82d25d2
Refactor implementation of operator+
heplesser Apr 18, 2024
8614a77
Implement faster element lookup for composite collections
heplesser Apr 18, 2024
1e51e92
Merge branch 'master' of github.com:nest/nest-simulator into fix_nc_s…
heplesser Apr 18, 2024
e41e5e3
Change node iteration in node_collection.cpp to avoid re-creating end()
heplesser Apr 19, 2024
7d46cd4
Revised comments for NodeCollection iteration
heplesser Apr 19, 2024
2619c3f
Try to fix artifact upload
heplesser Apr 19, 2024
0361d38
next attempt, include linux
heplesser Apr 19, 2024
32e603d
Fix abs path on macos
heplesser Apr 19, 2024
833ec10
More flexible approach to uploading artifacts
heplesser Apr 21, 2024
77b9a55
Make test log parsing robust
heplesser Apr 21, 2024
13742f2
Back to hardcoded artefact paths
heplesser Apr 21, 2024
e0b5ed4
Skip test_issue_3108.py tests if run with more than one MPI rank on G…
heplesser Apr 23, 2024
8faf24d
Hike Python version for macOS tests to 3.12, since macos-latest on Gi…
heplesser Apr 23, 2024
36e9b99
Fix typo in error message
heplesser May 23, 2024
54c68ff
Fix error in developer documentation of node collection slicing
heplesser May 23, 2024
4b63447
Merge branch 'master' of github.com:nest/nest-simulator into fix_nc_s…
heplesser May 23, 2024
6aad9d4
Fix pylint errors
heplesser May 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions libnestutil/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ set( nestutil_sources
lockptr.h
logging_event.h logging_event.cpp
logging.h
nest_types.h
numerics.h numerics.cpp
regula_falsi.h
sort.h
Expand Down
File renamed without changes.
144 changes: 144 additions & 0 deletions libnestutil/numerics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
*
*/

#include <cstdlib>
#include <iostream>
#include <numeric>

#include "nest_types.h"
#include "numerics.h"

#ifndef HAVE_M_E
Expand Down Expand Up @@ -126,3 +131,142 @@ is_integer( double n )
// factor 4 allows for two bits of rounding error
return frac_part < 4 * n * std::numeric_limits< double >::epsilon();
}

long
mod_inverse( long a, long m )
{
/*
The implementation here is based on the extended Euclidean algorithm which
solves

a x + m y = gcd( a, m ) = 1 mod m

for x and y. Note that the m y term is zero mod m, so the equation is equivalent
to

a x = 1 mod m

Since we only need x, we can ignore y and use just half of the algorithm.

We can assume without loss of generality that a < m, because if a = a' + j m with
0 < a' < m, we have

a x mod m = (a' + j m) x mod m = a' x + j x m mod m = a' x.

This implies that m ≥ 2.

For details on the algorithm, see D. E. Knuth, The Art of Computer Programming,
ch 4.5.2, Algorithm X (vol 2), and ch 1.2.1, Algorithm E (vol 1).
*/

assert( 0 < a );
assert( 2 <= m );

heplesser marked this conversation as resolved.
Show resolved Hide resolved
const long a_orig = a;
const long m_orig = m;

// If a ≥ m, the algorithm needs two extra rounds to transform this to
// a' < m, so we take care of this in a single step here.
a = a % m;

// Use half of extended Euclidean algorithm required to compute inverse
long s_0 = 1;
long s_1 = 0;

while ( a > 0 )
{
// get quotient and remainder in one go
const auto res = div( m, a );
m = a;
a = res.rem;

// line ordering matters here
const long s_0_new = -res.quot * s_0 + s_1;
s_1 = s_0;
s_0 = s_0_new;
}

// ensure positive result
s_1 = ( s_1 + m_orig ) % m_orig;

assert( m == 1 ); // gcd() == 1 required
assert( ( a_orig * s_1 ) % m_orig == 1 ); // self-test

return s_1;
}

size_t
first_index( long period, long phase0, long step, long phase )
{
assert( period > 0 );
assert( step > 0 );
assert( 0 <= phase0 and phase0 < period );
assert( 0 <= phase and phase < period );

/*
The implementation here is based on
https://math.stackexchange.com/questions/25390/how-to-find-the-inverse-modulo-m

We first need to solve

phase0 + k step = phase mod period
<=> k step = ( phase - phase0 ) = d_phase mod period

This has a solution iff d = gcd(step, period) divides d_phase.

Then, if d = 1, the solution is unique and given by

k' = mod_inv(step) * d_phase mod period

If d > 1, we need to divide the equation by it and solve

(step / d) k_0 = d_phase / d mod (period / d)

The set of solutions is then given by

k_j = k_0 + j * period / d for j = 0, 1, ..., d-1

and we need the smallest of these. Now we are interested in
an index given by k * step with a period of lcm(step, period)
(the outer_period below, marked by | in the illustration in
the doxygen comment), we immediately run over

k_j * step = k_0 * step + j * step * period / d mod lcm(step, period)

below and take the smallest. But since step * period / d = lcm(step, period),
the term in j above vanishes and k_0 * step mod lcm(step, period) is actually
the solution.

We do all calculations in signed long since we may encounter negative
values during the algorithm. The result will be non-negative and returned
as size_t. This is important because the "not found" case is signaled
by invalid_index, which is size_t.
*/

// This check is not only a convenience: If step == k * period, we only match if
// phase == phase0 and the algorithm below will fail if we did not return here
// immediately, because we get d == period -> period_d = 1 and modular inverse
// for modulus 1 makes no sense.
if ( phase == phase0 )
{
return 0;
}

const long d_phase = ( phase - phase0 + period ) % period;
const long d = std::gcd( step, period );

if ( d_phase % d != 0 )
{
return nest::invalid_index; // no solution exists
}

// Scale by GCD, since modular inverse requires gcd==1
const long period_d = period / d;
const long step_d = step / d;
const long d_phase_d = d_phase / d;

// Compute k_0 and multiply by step, see explanation in introductory comment
const long idx = ( d_phase_d * mod_inverse( step_d, period_d ) * step ) % std::lcm( period, step );

return static_cast< size_t >( idx );
}
48 changes: 48 additions & 0 deletions libnestutil/numerics.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,52 @@ double dtruncate( double );
*/
bool is_integer( double );

/**
* Returns inverse of integer a modulo m
*
* For integer a > 0, m ≥ 2, find x so that ( a * x ) mod m = 1.
*/
long mod_inverse( long a, long m );

/**
* Return first matching index for entry in container with double periodicity.
*
* Consider
* - a container of arbitrary length containing elements (e.g. GIDs) which map to certain values,
* e.g., VP(GID), with a given *period*, e.g., the number of virtual processes
* - that the phase (e.g., the VP number) of the first entry in the container is *phase0*
* - that we slice the container with a given *step*, causing a double periodicity
* - that we want to know the index of the first element in the container with a given *phase*,
* e.g., the first element on a given VP
*
* If such an index x exists, it is given by
*
* x = phase0 + k' mod lcm(period, step)
* k' = min_k ( phase0 + k step = phase mod period )
*
* As an example, consider
*
* idx 0 1 2 3 4 5 6 7 8 9 10 11 | 12 13 14 15 16 17 18
* gid 1 2 3 4 5 6 7 8 9 10 11 12 | 13 14 15 16 17 18 19
* vp 1 2 3 0 1 2 3 0 1 2 3 0 | 1 2 3 0 1 2 3
* * * * * | * * *
* 1 0 3 2 |
*
* Here, idx is a linear index into the container, gid neuron ids which map to the given vp numbers.
* The container is sliced with step=3, i.e., starting with the first element we take every third, as
* marked by the asterisks. The question is then at which index we find the first entry belonging to
* vp 0, 1, 2, or 3. The numbers in the bottom row show for clarity on which VP we find the respective
* asterisks. The | symbol marks where the pattern repeats itself.
*
* phase0 in this example is 1, i.e., the VP of the first element in the container.
*
* @note This function returns that index. It is the responsibility of the caller to check if the returned index
* is within the bounds of the actual container—the algorithm assumes an infinite container.
*
* @note The function returns *invalid_index* if no solution exists.
*
* See comments in the function definition for implementation details.
*/
size_t first_index( long period, long phase0, long step, long phase );

#endif
1 change: 0 additions & 1 deletion nestkernel/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ set ( nestkernel_sources
histentry.h histentry.cpp
model.h model.cpp
model_manager.h model_manager_impl.h model_manager.cpp
nest_types.h
nest_datums.h nest_datums.cpp
nest_names.cpp nest_names.h
nestmodule.h nestmodule.cpp
Expand Down
13 changes: 7 additions & 6 deletions nestkernel/conn_builder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ nest::OneToOneBuilder::connect_()
Node* target = n->get_node();

const size_t tnode_id = n->get_node_id();
const long lid = targets_->get_lid( tnode_id );
const long lid = targets_->get_nc_index( tnode_id );
if ( lid < 0 ) // Is local node in target list?
{
continue;
Expand Down Expand Up @@ -823,7 +823,7 @@ nest::AllToAllBuilder::connect_()
const size_t tnode_id = n->get_node_id();

// Is the local node in the targets list?
if ( targets_->get_lid( tnode_id ) < 0 )
if ( targets_->get_nc_index( tnode_id ) < 0 )
{
continue;
}
Expand Down Expand Up @@ -1106,7 +1106,7 @@ nest::FixedInDegreeBuilder::connect_()
const size_t tnode_id = n->get_node_id();

// Is the local node in the targets list?
if ( targets_->get_lid( tnode_id ) < 0 )
if ( targets_->get_nc_index( tnode_id ) < 0 )
{
continue;
}
Expand Down Expand Up @@ -1538,7 +1538,7 @@ nest::BernoulliBuilder::connect_()
const size_t tnode_id = n->get_node_id();

// Is the local node in the targets list?
if ( targets_->get_lid( tnode_id ) < 0 )
if ( targets_->get_nc_index( tnode_id ) < 0 )
{
continue;
}
Expand Down Expand Up @@ -1654,7 +1654,7 @@ nest::PoissonBuilder::connect_()
const size_t tnode_id = n->get_node_id();

// Is the local node in the targets list?
if ( targets_->get_lid( tnode_id ) < 0 )
if ( targets_->get_nc_index( tnode_id ) < 0 )
{
continue;
}
Expand Down Expand Up @@ -1847,7 +1847,8 @@ nest::TripartiteBernoulliWithPoolBuilder::connect_()
}
else
{
std::copy_n( third_->begin() + get_first_pool_index_( target.lid ), pool_size_, std::back_inserter( pool ) );
std::copy_n(
third_->begin() + get_first_pool_index_( target.nc_index ), pool_size_, std::back_inserter( pool ) );
}

// step 3, iterate through indegree to make connections for this target
Expand Down
2 changes: 0 additions & 2 deletions nestkernel/connection_creator.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ class ConnectionCreator
* - "mask": Mask definition (dictionary or masktype).
* - "kernel": Kernel definition (dictionary, parametertype, or double).
* - "synapse_model": The synapse model to use.
* - "targets": Which targets (model or lid) to select (dictionary).
* - "sources": Which targets (model or lid) to select (dictionary).
* - "weight": Synaptic weight (dictionary, parametertype, or double).
* - "delay": Synaptic delays (dictionary, parametertype, or double).
* - other parameters are interpreted as synapse parameters, and may
Expand Down
16 changes: 8 additions & 8 deletions nestkernel/connection_creator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ ConnectionCreator::pairwise_bernoulli_on_source_( Layer< D >& source,

if ( not tgt->is_proxy() )
{
const Position< D > target_pos = target.get_position( ( *tgt_it ).lid );
const Position< D > target_pos = target.get_position( ( *tgt_it ).nc_index );

if ( mask_.get() )
{
Expand Down Expand Up @@ -339,7 +339,7 @@ ConnectionCreator::pairwise_bernoulli_on_target_( Layer< D >& source,
const int thread_id = kernel().vp_manager.get_thread_id();
try
{
NodeCollection::const_iterator target_begin = target_nc->local_begin();
NodeCollection::const_iterator target_begin = target_nc->thread_local_begin();
NodeCollection::const_iterator target_end = target_nc->end();

for ( NodeCollection::const_iterator tgt_it = target_begin; tgt_it < target_end; ++tgt_it )
Expand All @@ -348,7 +348,7 @@ ConnectionCreator::pairwise_bernoulli_on_target_( Layer< D >& source,

assert( not tgt->is_proxy() );

const Position< D > target_pos = target.get_position( ( *tgt_it ).lid );
const Position< D > target_pos = target.get_position( ( *tgt_it ).nc_index );

if ( mask_.get() )
{
Expand Down Expand Up @@ -423,7 +423,7 @@ ConnectionCreator::pairwise_poisson_( Layer< D >& source,

if ( not tgt->is_proxy() )
{
const Position< D > target_pos = target.get_position( ( *tgt_it ).lid );
const Position< D > target_pos = target.get_position( ( *tgt_it ).nc_index );

if ( mask_.get() )
{
Expand Down Expand Up @@ -477,7 +477,7 @@ ConnectionCreator::fixed_indegree_( Layer< D >& source,
throw IllegalConnection( "Spatial Connect with fixed_indegree to devices is not possible." );
}

NodeCollection::const_iterator target_begin = target_nc->MPI_local_begin();
NodeCollection::const_iterator target_begin = target_nc->rank_local_begin();
NodeCollection::const_iterator target_end = target_nc->end();

// protect against connecting to devices without proxies
Expand All @@ -504,7 +504,7 @@ ConnectionCreator::fixed_indegree_( Layer< D >& source,

size_t target_thread = tgt->get_thread();
RngPtr rng = get_vp_specific_rng( target_thread );
Position< D > target_pos = target.get_position( ( *tgt_it ).lid );
Position< D > target_pos = target.get_position( ( *tgt_it ).nc_index );

// We create a source pos vector here that can be updated with the
// source position. This is done to avoid creating and destroying
Expand Down Expand Up @@ -637,7 +637,7 @@ ConnectionCreator::fixed_indegree_( Layer< D >& source,
Node* const tgt = kernel().node_manager.get_node_or_proxy( target_id );
size_t target_thread = tgt->get_thread();
RngPtr rng = get_vp_specific_rng( target_thread );
Position< D > target_pos = target.get_position( ( *tgt_it ).lid );
Position< D > target_pos = target.get_position( ( *tgt_it ).nc_index );

unsigned long target_number_connections = std::round( number_of_connections_->value( rng, tgt ) );

Expand Down Expand Up @@ -769,7 +769,7 @@ ConnectionCreator::fixed_outdegree_( Layer< D >& source,
throw IllegalConnection( "Spatial Connect with fixed_outdegree to devices is not possible." );
}

NodeCollection::const_iterator target_begin = target_nc->MPI_local_begin();
NodeCollection::const_iterator target_begin = target_nc->rank_local_begin();
NodeCollection::const_iterator target_end = target_nc->end();

for ( NodeCollection::const_iterator tgt_it = target_begin; tgt_it < target_end; ++tgt_it )
Expand Down