I3RemoveLargeDT - Reviewed by H. Dembinski 05/29/15
=================================================== 

URL: http://code.icecube.wisc.edu/svn/projects/sim-services/trunk/private/I3RemoveLargeDT.cxx

Repository UUID: 16731396-06f5-0310-8873-f7f720988828

Revision: 133136

Last Changed Author: hdembinski

Last Changed Rev: 133136

Last Changed Date: 2015-05-29 18:48:28 -0400 (Fri, 29 May 2015)

Documentation
*************
The auto-generated page on the web is missing:
http://software.icecube.wisc.edu/simulation_trunk/projects/sim-services/index.html

Basic documentation is available under resources/docs, but not registered by the "i3_project" call in CMakeLists.txt, the latter needs to be changed to::

    i3_project(sim-services
      PYTHON_DIR python
      PYTHON_DEST icecube/sim_services
      DOCS_DIR resources/docs)

Internal code documentation of functions/classes is ok.

I am missing an overall brief summary of what I3RemoveLargeDT is supposed to do.

Code
*************

I3RemoveLargeDT.cxx
+++++++++++++++++++

A central task in this module is to compute the minimum, maximum, and median of a range. I recommend to use the accumulator framework in boost for these tasks:
http://www.boost.org/doc/libs/1_58_0/doc/html/accumulators.html

Accumulators are descriptive, because they are high-level, and they do not require a temporary allocation of a vector for storage (in case of the median that is surprising, but they have an algorithm which approximates the median very well without storing a sorted range, I just glanced over the relevant paper).

Lines 120, 123, 181, 182

  These lines are very long. That makes it hard to read the code side-by-side with this code review. The latter two lines may become more readable, if they are broken at appropriate places.

Lines 129, 130, 167, 168

  Unusual initialization of doubles in "copy constructor" style. For PODs, the C-style assignment initialization is more intuitive, which is equivalent in terms of compiler instructions (and would use the copy constructor anyway if this were a class).

Lines 140, 170

  `BOOST_FOREACH` would also make these loop heads more readable.

Lines 132-136

  This comment is not correct. The median is eventually computed, not the mean.

Tests
+++++
http://software.icecube.wisc.edu/coverage/00_LATEST/sim-services/private/sim-services/I3RemoveLargeDT.cxx.gcov.html

According to the coverage report, this module is barely tested. This is odd, because resources/tests/remove_large_dt_pes.py seems to do reasonable in-situ testing, but it is not listed under "i3_test_scripts" in the CMakeLists.txt. This needs to be changed. I recommend to use wildcards for "i3_test_scripts". Kudos for the "All your base are belong to us" reference :).

- Lines - 1.6 %
- Functions - 16.7 %
- Branches - 0.6 %

Review Response 
+++++++++++++++
**A. Olivas 8/19/2015**

* All suggestions in the Documentation section have been addressed.  The code is integrated into the meta-project docs and there's a page about the module, with a good intro.
* Nearly all of the suggestions in Code have been addressed, with the exception of using boost::accumulators.  This is more of a wishlist item, but worth looking into, so it's a ticket for now and maybe it'll become a feature in a future release.
* The Test coverage is not pretty good. 90.6% line coverage and 91.7% function coverage.