Fix #489: correctly handling property of poisson distribution

matthiasrichter · sawenzel · commit e2bf35faa645 · 2017-08-17T13:43:05.000+02:00
- poisson distribution has two bins with equal probabilities at the
  maximum, checking now if the most abundant value is in one of those
  bins
- bugfix: consistently calculate value in bin center
  While the iterator was adding half of the step size in order to return
  center of the bin, the random function of the DataGenerator was missing
  this 0.5 term
- also replacing 'typedef's by 'using'
- fixing compilation warnings (mostly comparison of signed and unsigned)
diff --git a/Utilities/DataCompression/include/DataCompression/DataDeflater.h b/Utilities/DataCompression/include/DataCompression/DataDeflater.h
@@ -86,6 +86,8 @@ class DataDeflater {
   int reset() {
     mCurrent = 0;
     mFilledBits = 0;
+
+    return 0;
   }
 
   /**
@@ -169,7 +171,7 @@ class DataDeflater {
   /// current target word
   target_type mCurrent;
   /// current bit position
-  int mFilledBits;
+  unsigned mFilledBits;
   /// codec instance
   Codec mCodec;
 };
diff --git a/Utilities/DataCompression/include/DataCompression/dc_primitives.h b/Utilities/DataCompression/include/DataCompression/dc_primitives.h
@@ -139,10 +139,13 @@ class ExampleAlphabet {
   static bool isValid(value_type v);
 
   /// get index of value
-  static int getIndex(value_type symbol);
+  static unsigned getIndex(value_type symbol);
 
   /// get symbol from index
-  static value_type getSymbol(int index);
+  static value_type getSymbol(unsigned index);
+
+  /// get the range of indices aka number of indices
+  constexpr unsigned getIndexRange();
 
   typedef std::iterator<std::forward_iterator_tag, T> _iterator_base;
 
@@ -215,18 +218,24 @@ class ContiguousAlphabet {
   ///
   /// Each alphabet has to provide a one to one mapping of symbols to
   /// index values used for internal storage
-  static int getIndex(value_type symbol) {
+  /// For performance reasons, there is no range check
+  static unsigned getIndex(value_type symbol) {
     int index = symbol;
     if (_min < 0) index += -_min;
     else if (_min > 0) index -= _min;
     return index;
   }
 
   /// get symbol from index
-  static value_type getSymbol(int index) {
+  static value_type getSymbol(unsigned index) {
     return _min + index;
   }
 
+  /// get the range of indices aka number of indices
+  constexpr unsigned getIndexRange() {
+    return _max - _min;
+  }
+
   /// get the name of the alphabet
   ///
   /// name is part of the type definition, defined as a boost mpl string
diff --git a/Utilities/DataCompression/test/DataGenerator.h b/Utilities/DataCompression/test/DataGenerator.h
@@ -8,8 +8,6 @@
 // granted to it by virtue of its status as an Intergovernmental Organization
 // or submit itself to any jurisdiction.
 
-//-*- Mode: C++ -*-
-
 #ifndef DATAGENERATOR_H
 #define DATAGENERATOR_H
 //****************************************************************************
@@ -64,16 +62,17 @@ template<typename ValueT
          , typename ModelT>
 class DataGenerator {
 public:
-  typedef int size_type;
-  typedef ValueT result_type;
-  typedef DataGenerator self_type;
+  using size_type = std::size_t;
+  using value_type = ValueT;
+  using result_type = value_type;
+  using self_type = DataGenerator;
 
   template<typename... Args>
   DataGenerator(result_type _min,
                 result_type _max,
                 result_type _step,
                 Args&&... args)
-    : mGenerator(), min(_min), max(_max), step(_step), nbins((max-min)/step), mModel(std::forward<Args>(args)...) {}
+    : min(_min), max(_max), step(_step), nbins((max-min)/step), mGenerator(), mModel(std::forward<Args>(args)...) {}
   ~DataGenerator() {}
   DataGenerator(const DataGenerator&) = default;
   DataGenerator& operator=(const DataGenerator&) = default;
@@ -83,8 +82,7 @@ class DataGenerator {
   const result_type step;
   const size_type nbins;
 
-  typedef ValueT value_type;
-  typedef std::default_random_engine random_engine;
+  using random_engine = std::default_random_engine;
 
   /// get next random value
   // TODO: can it be const?
@@ -98,7 +96,7 @@ class DataGenerator {
       }
     }
     int bin = (v - min)/step;
-    return min + bin * step;
+    return min + bin * step + 0.5 * step;
   }
 
   /// get next random value
@@ -115,7 +113,7 @@ class DataGenerator {
     return mModel.getProbability(v);
   }
 
-  typedef std::iterator<std::forward_iterator_tag, result_type> _iterator_base;
+  using _iterator_base = std::iterator<std::forward_iterator_tag, result_type>;
 
   /**
    * @class iterator a forward iterator to access the bins
@@ -129,9 +127,9 @@ class DataGenerator {
     iterator(const ContainerT& parent, size_type count = 0) : mParent(parent), mCount(count) {}
     ~iterator() {}
 
-    typedef iterator self_type;
-    typedef typename _iterator_base::value_type value_type;
-    typedef typename _iterator_base::reference reference;
+    using self_type = iterator;
+    using value_type = typename _iterator_base::value_type;
+    using reference = typename _iterator_base::reference;
 
     // prefix increment
     self_type& operator++() {
@@ -153,7 +151,7 @@ class DataGenerator {
       return copy;
     }
 
-    value_type operator*() {return mParent.min + (mCount +.5) * mParent.step;}
+    value_type operator*() {return mParent.min + mCount * mParent.step + .5 * mParent.step;}
     //pointer operator->() const {return &mValue;}
     //reference operator[](size_type n) const;
 
@@ -194,7 +192,7 @@ template <class RealType = double
           >
 class normal_distribution : public _BASE {
 public:
-  typedef typename _BASE::result_type result_type;
+  using result_type = typename _BASE::result_type;
 
   normal_distribution(result_type _mean,
                       result_type _stddev
@@ -226,7 +224,7 @@ template <class IntType = int
           >
 class poisson_distribution : public _BASE {
 public:
-  typedef typename _BASE::result_type result_type;
+  using result_type = typename _BASE::result_type;
 
   poisson_distribution(result_type _mean) : _BASE(_mean), mean(_mean) {}
   ~poisson_distribution() {};
diff --git a/Utilities/DataCompression/test/Fifo.h b/Utilities/DataCompression/test/Fifo.h
@@ -58,7 +58,7 @@ template<class T
 class Fifo : protected _BASE {
  public:
   Fifo() : mMutex(), mFillStatus(), mStop(false) {}
-  typedef T value_type;
+  using value_type = T;
 
   /**
    * Push value to the FIFO
diff --git a/Utilities/DataCompression/test/test_DataDeflater.cxx b/Utilities/DataCompression/test/test_DataDeflater.cxx
@@ -46,7 +46,7 @@ namespace o2dc = o2::data_compression;
 template<typename DataContainerT, typename DeflatedDataT>
 bool compare(const DataContainerT& container, std::size_t bitwidth, const DeflatedDataT& targetBuffer)
 {
-  int wordcount = 0;
+  unsigned wordcount = 0;
   auto bufferWord = targetBuffer[wordcount];
   using target_type = typename DeflatedDataT::value_type;
   auto targetWidth = 8 * sizeof(target_type);
@@ -82,6 +82,8 @@ bool compare(const DataContainerT& container, std::size_t bitwidth, const Deflat
       length -= comparing;
     }
   }
+
+  return true;
 }
 
 BOOST_AUTO_TEST_CASE(test_DataDeflaterRaw)
diff --git a/Utilities/DataCompression/test/test_DataGenerator.cxx b/Utilities/DataCompression/test/test_DataGenerator.cxx
@@ -41,28 +41,27 @@ bool testWithDistribution(Args&&... args)
   o2::test::DataGenerator<value_type, DistributionType> dg(std::forward<Args>(args)...);
 
   std::vector<int> throws(dg.nbins);
-  const int nRolls = 1000000;
+  const unsigned nRolls = 1000000;
 
-  for (int n = 0; n < nRolls; n++) {
+  for (unsigned n = 0; n < nRolls; n++) {
     value_type v = dg();
-    int bin = v/dg.step - dg.min;
+    unsigned bin = v/dg.step - dg.min;
     BOOST_REQUIRE(bin < dg.nbins);
     throws[bin]++;
   }
 
-  value_type mostAbundantValue = dg.min;
+  int mostAbundantValueBin = 0;
   int mostAbundantValueCount = 0;
-  auto highestProbability = dg.getProbability(0);
-  highestProbability = 0.;
-  value_type mostProbableValue = dg.min;
+  auto highestProbability = dg.getProbability(dg.min);
+  highestProbability = 0;
   for (auto i : dg) {
     int bin = i/dg.step - dg.min;
+    BOOST_REQUIRE(bin >= 0);
     if (mostAbundantValueCount < throws[bin]) {
-      mostAbundantValue = i;
+      mostAbundantValueBin = bin;
       mostAbundantValueCount = throws[bin];
     }
     if (highestProbability < dg.getProbability(i)) {
-      mostProbableValue = i;
       highestProbability = dg.getProbability(i);
     }
     std::cout << std::setw(4)  << std::right << i << ": "
@@ -71,7 +70,14 @@ bool testWithDistribution(Args&&... args)
               << throws[bin]
               << std::endl;
   }
-  BOOST_CHECK(mostAbundantValue == mostProbableValue);
+  std::vector<int> mostProbableValueBins;
+  for (auto i : dg) {
+    int bin = i/dg.step - dg.min;
+    if (dg.getProbability(i) >= highestProbability) {
+      mostProbableValueBins.push_back(bin);
+    }
+  }
+  BOOST_CHECK(std::find(mostProbableValueBins.begin(), mostProbableValueBins.end(), mostAbundantValueBin) != mostProbableValueBins.end());
 
   return true;
 }
diff --git a/Utilities/DataCompression/test/test_HuffmanCodec.cxx b/Utilities/DataCompression/test/test_HuffmanCodec.cxx
@@ -77,10 +77,10 @@ BOOST_AUTO_TEST_CASE(test_HuffmanCodec)
   // in the range [-7, 10] including the upper bound
   // the first definition is a data type, then an object of this type is
   // defined
-  typedef o2::test::normal_distribution<double> TestDistribution_t;
-  typedef o2::test::DataGenerator<int16_t, TestDistribution_t> DataGenerator_t;
-  DataGenerator_t dg(-7.5, 10.5, 1., 0., 1.);
-  typedef ContiguousAlphabet<DataGenerator_t::value_type, -7, 10> SimpleRangeAlphabet_t;
+  using TestDistribution_t = o2::test::normal_distribution<double>;
+  using DataGenerator_t = o2::test::DataGenerator<int16_t, TestDistribution_t>;
+  DataGenerator_t dg(-7, 10, 1, 0., 1.);
+  using SimpleRangeAlphabet_t = ContiguousAlphabet<DataGenerator_t::value_type, -7, 10>;
   SimpleRangeAlphabet_t alphabet;
 
   ////////////////////////////////////////////////////////////////////////////
@@ -90,11 +90,11 @@ BOOST_AUTO_TEST_CASE(test_HuffmanCodec)
   // the node type is defined to be HuffmanNode specialized to bitset<16>
   // third template parameter determines whether code has to be decoded
   // MSB to LSB (true) or LSB to MSB (false)
-  typedef o2::HuffmanModel<
+  using HuffmanModel_t = o2::HuffmanModel<
     ProbabilityModel<SimpleRangeAlphabet_t>
     , o2::HuffmanNode<std::bitset<32> >
     , true
-    > HuffmanModel_t;
+    >;
   HuffmanModel_t huffmanmodel;
 
   std::cout << std::endl << "Huffman probability model after initialization: " << std::endl;
@@ -104,7 +104,6 @@ BOOST_AUTO_TEST_CASE(test_HuffmanCodec)
   }
 
   // add probabilities from data generator as weights for every symbol
-  int x = 0;
   for (auto s : alphabet) {
     huffmanmodel.addWeight(s, dg.getProbability(s));
   }
@@ -123,7 +122,7 @@ BOOST_AUTO_TEST_CASE(test_HuffmanCodec)
   std::cout << std::endl << "Generating binary tree and Huffman codes" << std::endl;
   huffmanmodel.GenerateHuffmanTree();
   huffmanmodel.print();
-  typedef o2::HuffmanCodec<HuffmanModel_t > Codec_t;
+  using Codec_t = o2::HuffmanCodec<HuffmanModel_t >;
   Codec_t codec(huffmanmodel);
 
   ////////////////////////////////////////////////////////////////////////////
@@ -161,7 +160,7 @@ BOOST_AUTO_TEST_CASE(test_HuffmanCodec)
   o2::test::Fifo<DataGenerator_t::value_type> fifoRandvals;
 
   // FIFO for encoded values
-  typedef o2::test::Fifo<uint32_t> FifoBuffer_t;
+  using FifoBuffer_t = o2::test::Fifo<uint32_t>;
   FifoBuffer_t fifoEncoded;
 
   const int nRolls = 1000000;
diff --git a/Utilities/DataCompression/test/test_dc_primitives.cxx b/Utilities/DataCompression/test/test_dc_primitives.cxx
@@ -63,7 +63,7 @@ BOOST_AUTO_TEST_CASE(test_dc_primitives)
 {
   // test the getmax meta program
   std::cout << std::endl << "Testing getmax meta program ..." << std::endl;
-  typedef boost::mpl::vector_c<uint16_t, 0, 1, 2, 3, 4, 31, 32, 64> bitranges;
+  using bitranges = boost::mpl::vector_c<uint16_t, 0, 1, 2, 3, 4, 31, 32, 64>;
   boost::mpl::for_each<bitranges, boost::type<boost::mpl::_> >(getmaxTester());
 
   // test the getnofelements meta program
@@ -82,15 +82,15 @@ BOOST_AUTO_TEST_CASE(test_dc_primitives)
   std::cout << std::endl << "Testing alphabet template ..." << std::endl;
   // declare two types of alphabets: a contiguous range alphabet with symbols
   // between -1 and 10 and a bit-range alphabet for a 10-bit word
-  typedef boost::mpl::string<'T','e','s','t'>::type TestAlphabetName;
-  typedef boost::mpl::string<'1','0','-','b','i','t'>::type TenBitAlphabetName;
-  typedef ContiguousAlphabet<int16_t, -1, 10, TestAlphabetName> TestAlphabet;
-  typedef BitRangeContiguousAlphabet<int16_t, 10, TenBitAlphabetName> TenBitAlphabet;
+  using TestAlphabetName = boost::mpl::string<'T','e','s','t'>::type;
+  using TenBitAlphabetName = boost::mpl::string<'1','0','-','b','i','t'>::type;
+  using TestAlphabet = ContiguousAlphabet<int16_t, -1, 10, TestAlphabetName>;
+  using TenBitAlphabet = BitRangeContiguousAlphabet<int16_t, 10, TenBitAlphabetName>;
 
   // now check a set of values if they are valid in each of the alphabets
   // the check is done at runtime on types of alphabets rather than on
   // actual objects
   std::vector<int16_t> values = {0 , 5, 15, -2, -1};
-  typedef boost::mpl::vector<TestAlphabet, TenBitAlphabet> ParameterSet;
+  using ParameterSet = boost::mpl::vector<TestAlphabet, TenBitAlphabet>;
   boost::mpl::for_each<ParameterSet>( AlphabetTester<std::vector<int16_t>>(values) );
 }

Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ namespace o2dc = o2::data_compression;`
`46`	`46`	`template<typename DataContainerT, typename DeflatedDataT>`
`47`	`47`	`bool compare(const DataContainerT& container, std::size_t bitwidth, const DeflatedDataT& targetBuffer)`
`48`	`48`	`{`
`49`		`- int wordcount = 0;`
	`49`	`+ unsigned wordcount = 0;`
`50`	`50`	`auto bufferWord = targetBuffer[wordcount];`
`51`	`51`	`using target_type = typename DeflatedDataT::value_type;`
`52`	`52`	`auto targetWidth = 8 * sizeof(target_type);`
`@@ -82,6 +82,8 @@ bool compare(const DataContainerT& container, std::size_t bitwidth, const Deflat`
`82`	`82`	`length -= comparing;`
`83`	`83`	`}`
`84`	`84`	`}`
	`85`	`+`
	`86`	`+ return true;`
`85`	`87`	`}`
`86`	`88`
`87`	`89`	`BOOST_AUTO_TEST_CASE(test_DataDeflaterRaw)`