[ML] Fix for CPoissonMeanConjugate sampling error. (#335) (#338)

Fix for "sample out of bounds" error generated from
CPoissonMeanConjugate

Backports #335

Author: edsavage

docs/CHANGELOG.asciidoc

@@ -28,6 +28,22 @@
 
 //=== Regressions
 
+ == {es} version 6.6.0
+
+=== Breaking Changes
+
+=== Deprecations
+
+=== New Features
+
+=== Enhancements
+
+=== Bug Fixes
+
+Fix cause of "Sample out of bounds" error message (See {ml-pull}355[355].}
+
+=== Regressions
+
  == {es} version 6.5.3
 
 === Bug Fixes

lib/maths/CPoissonMeanConjugate.cc

@@ -646,6 +646,9 @@ void CPoissonMeanConjugate::sampleMarginalLikelihood(std::size_t numberSamples,
                 // Sanity check the sample: should be in the distribution support.
                 if (sample >= support.first && sample <= support.second) {
                     samples.push_back(sample);
+                } else if (sample < support.first) {
+                    // Extremely rarely the variance is large and the offset is 0, resulting in a negative sample.
+                    // We discard these negative samples as they are logically incongruous for a counting distribution.
                 } else {
                     LOG_ERROR(<< "Sample out of bounds: sample = " << sample << ", support = ["
                               << support.first << "," << support.second << "]"

lib/maths/unittest/CPoissonMeanConjugateTest.cc

@@ -494,6 +494,54 @@ void CPoissonMeanConjugateTest::testSampleMarginalLikelihood() {
     }
 }
 
+void CPoissonMeanConjugateTest::testSampleMarginalLikelihoodInSupportBounds() {
+    // Here we test that the retrieved samples are
+    // a) positive
+    // b) monotonically increasing
+    // c) No ERROR log messages are generated
+    // The input sample and weight are chosen to be ones that generate
+    // an initial sample that is less than an offset value of 0.0
+
+    const char* logFile = "test.log";
+
+    std::remove(logFile);
+    // log at level ERROR only
+    CPPUNIT_ASSERT(ml::core::CLogger::instance().reconfigureFromFile(
+        "testfiles/testLogErrorsLog4cxx.properties"));
+
+    CPoissonMeanConjugate filter(CPoissonMeanConjugate::nonInformativePrior());
+
+    TDouble1Vec sampleVec(1, 283.3232);
+    maths_t::TDoubleWeightsAry1Vec weightVec(1, maths_t::countWeight(0.0206505));
+
+    filter.addSamples(sampleVec, weightVec);
+
+    std::size_t numberSampled = 50u;
+    TDouble1Vec sampled;
+
+    filter.sampleMarginalLikelihood(numberSampled, sampled);
+
+    double prevSample{0.0};
+    for (double sample : sampled) {
+        CPPUNIT_ASSERT(sample > prevSample);
+        CPPUNIT_ASSERT(sample >= 0.0);
+        prevSample = sample;
+    }
+
+    // Revert to the default properties for the test framework - very similar to the hardcoded default.
+    CPPUNIT_ASSERT(ml::core::CLogger::instance().reconfigureFromFile("testfiles/log4cxx.properties"));
+
+    std::ifstream log(logFile);
+    CPPUNIT_ASSERT(log.is_open());
+    char line[256];
+    while (log.getline(line, 256)) {
+        LOG_INFO(<< "Got '" << line << "'");
+        CPPUNIT_ASSERT(false);
+    }
+    log.close();
+    std::remove(logFile);
+}
+
 void CPoissonMeanConjugateTest::testCdf() {
     // Test error cases.
     //
@@ -960,6 +1008,9 @@ CppUnit::Test* CPoissonMeanConjugateTest::suite() {
         &CPoissonMeanConjugateTest::testSampleMarginalLikelihood));
     suiteOfTests->addTest(new CppUnit::TestCaller<CPoissonMeanConjugateTest>(
         "CPoissonMeanConjugateTest::testCdf", &CPoissonMeanConjugateTest::testCdf));
+    suiteOfTests->addTest(new CppUnit::TestCaller<CPoissonMeanConjugateTest>(
+        "CPoissonMeanConjugateTest::testSampleMarginalLikelihoodInSupportBounds",
+        &CPoissonMeanConjugateTest::testSampleMarginalLikelihoodInSupportBounds));
     suiteOfTests->addTest(new CppUnit::TestCaller<CPoissonMeanConjugateTest>(
         "CPoissonMeanConjugateTest::testProbabilityOfLessLikelySamples",
         &CPoissonMeanConjugateTest::testProbabilityOfLessLikelySamples));

lib/maths/unittest/CPoissonMeanConjugateTest.h

@@ -15,6 +15,7 @@ class CPoissonMeanConjugateTest : public CppUnit::TestFixture {
     void testPropagation();
     void testMeanEstimation();
     void testMarginalLikelihood();
+    void testSampleMarginalLikelihoodInSupportBounds();
     void testMarginalLikelihoodMode();
     void testMarginalLikelihoodVariance();
     void testSampleMarginalLikelihood();

lib/maths/unittest/testfiles/log4cxx.properties

@@ -0,0 +1,10 @@
+# Set root logger level to DEBUG and its only appender to A1.
+log4j.rootLogger=DEBUG, A1
+
+# A1 is set to be a ConsoleAppender.
+log4j.appender.A1=org.apache.log4j.ConsoleAppender
+log4j.appender.A1.Target=System.err
+
+# A1 uses PatternLayout.
+log4j.appender.A1.layout=org.apache.log4j.PatternLayout
+log4j.appender.A1.layout.ConversionPattern=%d %d{%Z} [%P] %-5p %F@%L %m%n

lib/maths/unittest/testfiles/testLogErrorsLog4cxx.properties

@@ -0,0 +1,20 @@
+# Set root logger level to INFO and its only appender to A1.
+log4j.rootLogger=DEBUG, A1
+
+# Also set up a logger named after the log identifier of the program, with
+# appender A2.
+log4j.logger.%N=ERROR, A2
+
+# A1 is set to be a ConsoleAppender.
+log4j.appender.A1=org.apache.log4j.ConsoleAppender
+log4j.appender.A1.Target=System.err
+log4j.appender.A1.layout=org.apache.log4j.PatternLayout
+log4j.appender.A1.layout.ConversionPattern=%d %d{%Z} [%P] %-5p %F@%L %m%n
+
+# A2 is set to log to a rolling file in a current directory.
+log4j.appender.A2=org.apache.log4j.RollingFileAppender
+log4j.appender.A2.File=test.log
+log4j.appender.A2.MaxFileSize=10MB
+log4j.appender.A2.MaxBackupIndex=1
+log4j.appender.A2.layout=org.apache.log4j.PatternLayout
+log4j.appender.A2.layout.ConversionPattern=%d %d{%Z} [%P] %-5p %F@%L %m%n