Improve BlockVector to fix issues when sorting with recent versions of Boost #1502

hakonsbm · 2020-04-06T14:48:33Z

Until Boost version 1.69.0, spreadsort used std::sort for small lists (<1000 elements). But with 1.69.0 and onward it uses pdqsort from the Boost library instead. Because the pdqsort algorithm uses some operators that neither std::sort nor spreadsort uses, and because of inadequacies in the BlockVector implementation, this caused lists to not be sorted.

When sorting with spreadsort from the Boost library, iterators for the BlockVector to be sorted and the BlockVector on which the same operations are performed, are combined. The combination is implemented with the IteratorPair iterator, which uses iterator_facade from the Boost library. The iterator_facade uses a few core functions to infer operators of the iterator.

The problem arose when pdqsort used the operator operator-=(). In IteratorPair, that operator is inferred from the function

advance( n ) 
{ 
  sort_iter_ += n; 
  perm_iter_ += n; 
}

Where sort_iter_ and perm_iter_ are iterators for the two BlockVectors. However, for operator-=(), advance(n) is called with a negative n. The iterator of BlockVector didn't take into consideration that its operator+=(n) could be called with a negative n. This PR improves BlockVector and its iterator to take that into consideration, solving the problem.

In addition, this PR

adds additional C++ tests of BlockVector
switches to the single header variant for Boost tests, so that NEST can be installed with Boost without static libraries
moves to the latest Boost version (1.72.0) on Travis

Fixes #1489
Fixes #1239

To be able to use header-only installation of Boost, without static libraries.

heplesser

@hakonsbm Thanks for working this out! I have a few questions and suggestions. In some places I am probably just not seeing through things properly ;).

heplesser · 2020-04-07T20:55:43Z

cmake/ProcessOptions.cmake

@@ -594,6 +596,7 @@ function( NEST_PROCESS_WITH_BOOST )
      set( BOOST_LIBRARIES "${Boost_LIBRARIES}" PARENT_SCOPE )
      set( BOOST_INCLUDE_DIR "${Boost_INCLUDE_DIR}" PARENT_SCOPE )
      set( BOOST_VERSION "${Boost_MAJOR_VERSION}.${Boost_MINOR_VERSION}.${Boost_SUBMINOR_VERSION}" PARENT_SCOPE )
+      include_directories( ${Boost_INCLUDE_DIRS} )


Just a little confused here: On line 597, we have BOOST_INCLUDE_DIR and Boost_INCLUDE_DIR, and here we have Boost_INCLUDE_DIRS (plural). Is this all as it should be, or is there some typo?

Both Boost_INCLUDE_DIR and Boost_INCLUDE_DIRS will probably point to the same location. However, based on the examples in the documentation, and this answer in the CMake forum, Boost_INCLUDE_DIRS is the correct variable to use.

I have just updated PR #1477 where I used,
include_directories( ${Boost_INCLUDE_DIR} )

should I change to
include_directories( ${Boost_INCLUDE_DIRS} )
and use
set( BOOST_INCLUDE_DIR "${Boost_INCLUDE_DIRS}" PARENT_SCOPE )
as commented by @hakonsbm ?

And just to add, BOOST_INCLUDE_DIR is a variable set on the scope above the current (PARENT_SCOPE, see manual), and the following code would not work as expected, as it will contain the previous value of tha variable.
include_directories( ${BOOST_INCLUDE_DIR} )

heplesser · 2020-04-07T21:00:48Z

libnestutil/block_vector.h

@@ -247,6 +260,9 @@ class BlockVector
   */
  int get_max_block_size() const;

+  // TODO: To make BlockVector a complete random access container, it should also implement
+  // max_size(), rbegin(), and rend().


This should either be done within this PR if straightforward or turned into a follow-up issue, but not stay as a "TODO" comment. If turned into a follow-up issue, would it be an idea to implement these three methods now to just throw an exception?

I have created function definitions that throw exceptions and removed the "TODO".

heplesser · 2020-04-07T21:08:45Z

libnestutil/block_vector.h

@@ -606,6 +640,13 @@ operator-( const const_iterator& other ) const
  return ( block_index_ - other.block_index_ ) * max_block_size + ( this_element_index - other_element_index );
 }

+template < typename value_type_, typename ref_, typename ptr_ >
+inline typename bv_iterator< value_type_, ref_, ptr_ >::reference bv_iterator< value_type_, ref_, ptr_ >::operator[](


I am a bit confused: Stripping the template arguments, this looks to me like bv_iterator::operator[]. But do we really want to apply array indexing to the iterator? I'd expected it used on the container. But I may be overlooking something here.

For iterators, the operator[] is the offset dereference operator, which gets the dereferenced element shifted n places relative to the iterator position. It is required for random access iterators.

heplesser · 2020-04-07T21:09:44Z

testsuite/cpptests/test_block_vector.h

+      reference.push_back( i );
+    }
+  }
+  ~bv_vec_reference_fixture()


Insert empty line above

heplesser · 2020-04-07T21:13:55Z

testsuite/cpptests/test_block_vector.h

@@ -260,6 +283,119 @@ BOOST_AUTO_TEST_CASE( test_iterator_compare )
  BOOST_REQUIRE( not( it_b < it_a ) );


Would it make sense to add tests for all comparison operators here?

heplesser · 2020-04-07T21:19:10Z

testsuite/cpptests/test_sort.h

-  }
+  BOOST_REQUIRE( std::is_sorted( bv_sort.begin(), bv_sort.end() ) );
+  BOOST_REQUIRE( std::is_sorted( bv_perm.begin(), bv_perm.end() ) );
+


Could you add a comment explaining why you perform both the is_sorted and the equal tests?

heplesser · 2020-04-07T21:22:00Z

testsuite/cpptests/test_sort.h

@@ -27,78 +27,151 @@
 #include <boost/test/unit_test.hpp>

 // C++ includes:
+#include <algorithm>
 #include <vector>

 // Includes from libnestutil:
 #include "sort.h"

 namespace nest


Why is everything put in namespace nest?

heplesser · 2020-04-07T21:23:09Z

testsuite/cpptests/test_sort.h


 BOOST_AUTO_TEST_SUITE( test_sort )

 /**
 * Tests whether two arrays with randomly generated numbers are sorted
- * correctly by a single call to sort.
+ * correctly when sorting with the built-in quicksort.


"built-in" == NEST's own?

heplesser · 2020-04-07T21:32:35Z

testsuite/cpptests/test_sort.h

-  nest_quicksort( bv0, bv1 );
+/**
+ * Tests whether two arrays with randomly generated numbers are sorted
+ * correctly when sorting with Boost.


We know that Boost switches sort algorithms between small and large containers, with a boundary at 1000 elements according to Boost::sort documentation (see boost::sort::spreadsort::detail::min_sort_size defined in boost/sort/spreadsort/detail/constants.hpp). The tests here should cover both cases.

- Added tests of all BlockVector iterator compararators - Add testing of sorting with small data sets

hakonsbm · 2020-04-14T12:07:49Z

@heplesser Thanks for your review! I have addressed your comments, please have another look.

heplesser

@hakonsbm Nicely done!

gtrensch

Nice work!
Please see my comment regarding downloading boost in travis_build.sh.

gtrensch · 2020-04-15T18:16:49Z

extras/travis_build.sh

+cp -fr boost_1_72_0 $HOME/.cache
+rm -fr boost_1_72_0
+CONFIGURE_BOOST="-Dwith-boost=$HOME/.cache/boost_1_72_0"
+


The above handles the boost library differently than, for example, music or sionlib. Is there a specific reason for this? If not, wouldn't it be better to implement this the same way? I would suggest to hide the above in a file extras/install_boost.sh, adjust .travis.yaml accordingly and wrap boost with an environment variable, same as it is for all other libraries.

@gtrensch The idea was to always compile with Boost, like Travis currently does. But I can implement it in a way similar to the other libraries. Should some Travis jobs not compile with Boost?

@hakonsbm The static code check (the first build configuration in the travis build matrix) does not require libboost. I will make a suggestion for a change and create a PR against your branch.

@hakonsbm I have created the PR, please check.

Handle libboost same as all other libraries

gtrensch

Thanks for the contribution! Approving!

Backport of #1502 to 2.20.1

that version, see nest#1502.

hakonsbm added 5 commits April 6, 2020 10:33

Fix issue when passing a negative value to operator+= and operator-=

0bdcf0f

Added and refined BlockVector operators

c07f764

Improved and added additional C++ tests

d6c1fb1

Using single header variant for Boost tests

3b57c96

To be able to use header-only installation of Boost, without static libraries.

Readded missing fi

c180b16

heplesser assigned hakonsbm Apr 7, 2020

heplesser added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation labels Apr 7, 2020

heplesser added this to the NEST 2.20.1 milestone Apr 7, 2020

heplesser self-requested a review April 7, 2020 08:39

H27CK mentioned this pull request Apr 7, 2020

Segfault with Boost => 1.69 #1506

Closed

heplesser requested changes Apr 7, 2020

View reviewed changes

heplesser requested review from gtrensch and jougs and removed request for jougs April 7, 2020 21:38

clinssen mentioned this pull request Apr 9, 2020

Using NESTML with repo-based NEST and Boost nest/nestml#542

Closed

hakonsbm added 4 commits April 14, 2020 11:44

Replaced TODO with function definitions without implementations

31a596b

Improved C++ tests

4259245

- Added tests of all BlockVector iterator compararators - Add testing of sorting with small data sets

Removed C++ test from nest namespace

8e0d24e

Changed Travis Boost installation from move to forced copy and remove

cf7d6a8

heplesser approved these changes Apr 14, 2020

View reviewed changes

heplesser mentioned this pull request Apr 15, 2020

Build NEST with Boost as default #1477

Merged

gtrensch requested changes Apr 15, 2020

View reviewed changes

handle libboost same as all other libraries

6de0daa

gtrensch mentioned this pull request Apr 20, 2020

Handle libboost same as all other libraries hakonsbm/nest-simulator#29

Merged

heplesser mentioned this pull request Apr 20, 2020

Use vector of integer for per-thread bool indicators (backport) #1455

Merged

constistent directory name for boost install directory

7d62091

niltonlk added a commit to niltonlk/nest-simulator that referenced this pull request Apr 20, 2020

Correct include_directories accordingly to changes made on PR nest#1502.

450fb47

Merge pull request #29 from gtrensch/gtrensch_boost_blockvector_fix

a0d278c

Handle libboost same as all other libraries

gtrensch approved these changes Apr 22, 2020

View reviewed changes

hakonsbm mentioned this pull request Apr 22, 2020

Final updates in documentation and the debian package for v2.20.1 #1472

Merged

Merge remote-tracking branch 'nest/master' into boost_blockvector_fix

31402cf

heplesser merged commit d3d9a4a into nest:master Apr 23, 2020

jasperalbers mentioned this pull request Apr 24, 2020

Improve BlockVector to fix issues when sorting with recent versions of Boost (backport) #1551

Merged

hakonsbm added a commit that referenced this pull request Apr 24, 2020

Merge pull request #1551 from jasperalbers/nest-2.20.1-backport-PR-#1502

d490f25

Backport of #1502 to 2.20.1

hakonsbm deleted the boost_blockvector_fix branch May 18, 2020 07:35

hakonsbm mentioned this pull request Nov 13, 2020

Modernize random number generator setup #1549

Merged

16 tasks

hakonsbm mentioned this pull request Jan 14, 2021

segmentation fault for slightly larger networks, related to version with openmpi #1881

Closed

heplesser added a commit to heplesser/nest-simulator that referenced this pull request Feb 23, 2021

Hike Boost minimum version to 1.69 due to change in sort algorithm with

3cce5a8

that version, see nest#1502.

heplesser mentioned this pull request Feb 23, 2021

Hike Boost minimum version to 1.69 #1937

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve BlockVector to fix issues when sorting with recent versions of Boost #1502

Improve BlockVector to fix issues when sorting with recent versions of Boost #1502

hakonsbm commented Apr 6, 2020 •

edited

Loading

heplesser left a comment

heplesser Apr 7, 2020

hakonsbm Apr 14, 2020

niltonlk Apr 20, 2020

heplesser Apr 7, 2020

hakonsbm Apr 14, 2020 •

edited

Loading

heplesser Apr 7, 2020

hakonsbm Apr 14, 2020

heplesser Apr 7, 2020

heplesser Apr 7, 2020

heplesser Apr 7, 2020

heplesser Apr 7, 2020

heplesser Apr 7, 2020

heplesser Apr 7, 2020

hakonsbm commented Apr 14, 2020

heplesser left a comment

gtrensch left a comment

gtrensch Apr 15, 2020

hakonsbm Apr 16, 2020

gtrensch Apr 20, 2020

gtrensch Apr 20, 2020

gtrensch left a comment

		@@ -260,6 +283,119 @@ BOOST_AUTO_TEST_CASE( test_iterator_compare )
		BOOST_REQUIRE( not( it_b < it_a ) );

Improve BlockVector to fix issues when sorting with recent versions of Boost #1502

Improve BlockVector to fix issues when sorting with recent versions of Boost #1502

Conversation

hakonsbm commented Apr 6, 2020 • edited Loading

heplesser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakonsbm Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakonsbm commented Apr 14, 2020

heplesser left a comment

Choose a reason for hiding this comment

gtrensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtrensch left a comment

Choose a reason for hiding this comment

hakonsbm commented Apr 6, 2020 •

edited

Loading

hakonsbm Apr 14, 2020 •

edited

Loading