Code review of rpdf

Reviewer: J. Braun SVN Revision: 162804 Date: May 29, 2018

Overview

rpdf was created by Kevin Meagher to address code quality issues with ipdf. The issues were presented by Kevin:

https://drive.google.com/drive/folders/1Vcr4x9Jmh8EaNeZYuT8Wm2dubaIBo6bG

rpdf consists mostly of I3RecoLLH, a new gulliver service for reconstructions using Pandel likelihoods, and algorithms copied mostly verbatim from ipdf.

Overall Impression

The code in rpdf represents a major improvement over ipdf. A few minor issues need to be addressed before rpdf is used in standard processing:

• Unit tests must be written (see below)

• Speed and accuracy should be tested against ipdf and documented

• Where appropriate, references to ipdf should be placed in the code so that users can find the SVN history

Project Metrics

Files (including directories):

rpdf:    33
ipdf:    119

Lines of code:

         rpdf    ipdf
         ----    ----
   .h:    576    6966
 .cxx:   1196    4980
  .py:    206     174
 --------------------
total:   1978   12120

Lines of .rst documentation:

rpdf:   566
ipdf:   653

Unit tests:

rpdf: 0
ipdf: 79

Comments: rpdf contains only 15% of the lines of code contained in ipdf. If rpdf satisfies all current usage of ipdf, this is a significant success. Documentation and unit tests will be discussed below.

Documentation

The documentation in rpdf is much more thorough than the previous documentation in ipdf. The improvements include background on likelihood reconstruction, derivation on geometrical calculations, motivation for the Pandel PDF, description of the convoluted Pandel PDF, and images from the AMANDA reconstruction paper and convoluted Pandel paper.

Unit Tests

rpdf contains a single python file in resources/test that compares ipdf reconstruction results against rpdf. The file is not automatically run by ‘make test’. In contrast, ipdf contains 79 unit tests. While many of these tests may not be relevant for rpdf, I believe at least some minimum test coverage is warranted before replacing ipdf.

Examples

rpdf/resources/examples contains two Python files. python_I3RecoLLH.py demonstrates how to obtain PDF values from rpdf, and python_likelihood_service.py demonstrates how to use rpdf in an IceTray module and how to create a new gulliver likelihood service. These examples are sufficient in my view.

Code

The code is high quality and much cleaner than that of ipdf. ipdf uses CRTP, with child classes containing the PDF implementation and access to the PDF done through the base class, whereas rpdf stores a pointer to the selected PDF function. Additionally, ipdf uses a template parameter for the ice model, whereas rpdf uses a struct. Favoring composition over templates and an inheritance hierarchy greatly improves the readability of the code. Additionally, the removal of old PDFs we no longer use is welcome.

The structure of the project is straightforward. Inline comments and method/class comments are generally very good. Issues with individual files are generally stylistic. I recommend running a spelling check.

Issues with specific files:

private/rpdf/I3RecoLLH.cxx: - Inconsistent tabs/spaces used in indentation - Indented braces in ‘for’ blocks is strongly discouraged

private/rpdf/I3RecoLLHFactory.cxx: - Inconsistent tabs/spaces used in indentation - Indented braces are strongly discouraged. The following:

if (type=="IceModel")
  {
    ice_model_=boost::python::extract<rpdf::IceModel>(icemodel);
  }

should be either

if (type=="IceModel")
{
  ice_model_=boost::python::extract<rpdf::IceModel>(icemodel);
}

or

if (type=="IceModel") {
  ice_model_=boost::python::extract<rpdf::IceModel>(icemodel);
}

private/rpdf/pandel.cxx:

• Inconsistent tabs/spaces used in indentation

• Many stylistic issues in this file that are carried over from ipdf. Not all of these need to be listed or fixed, but at least the missing spaces should be fixed, as this impacts readability.

• References to ipdf should be placed in the code so that users can find the SVN history