Code Review

# Code Review

Reviewer: Claudio Kopper <ckopper@icecube.aq>

This is a code review for LeptonInjector SVN r179409 found at
https://code.icecube.wisc.edu/svn/meta-projects/combo/branches/snobo/LeptonInjector/?p=179409 .

Guidelines used are from https://docs.icecube.aq/combo/trunk/general/code_reviews.html .

First glance:

* The project comes with an example script in resources/examples/inject_muons.py
* It includes a bare-bones README file for documentation in resources/doc/README
* There is currently no further documentation as part of the project.
* Several C++-based tests are included.

Source Code:

* The directory structure follows the guidelines laid out int the code review
  guidelines. LeptonInjector does use a single public header `LeptonInjector.h`
* The project is laid out as a base class `LeptonInjectorBase` (drived
  from `I3ConditionalModule` as expected) from which two classes for general
  use are derived:
  - `RangedLeptonInjector`
  - `VolumeLeptonInjector`
  - `MultiLeptonInjector` (which seems to combine "Ranged" and "Volume" modes.)
* Each injector I3Module has associated configuration objects derived from their
  own common base class `BasicInjectionConfiguration` (a I3FrameObject):
  - `RangedInjectionConfiguration`
  - `VolumeInjectionConfiguration`
  - `MinimalInjectionConfiguration` (associated with `MultiLeptonInjector`)
* There are several utility functions and modules collected in the same header
  file. Everything is collected into a single namespace called `LeptonInjector`.
* Event properties are stored in `I3FrameObject`-derived objects of type
  `RangedEventProperties` or `VolumeEventProperties` (both derived from
  `BasicEventProperties`.)

* There seems to be functionality to store configuration objects in hdf5 files
  (I note that they are also frame objects). This is in `private/LeptonInjector/h5write.cxx`
  which currently includes some dead code in `/* */` blocks which should be
  removed.
  - The hdf5 code is only able to write `RangedInjectionConfiguration`
    and `VolumeInjectionConfiguration` objects. No provision to store/write
    `MinimalInjectionConfiguration` objects seems to exist. It is unclear why/if
    these objects would not need to be serialized.
    
* I3/boost serialization code seems to exist in `private/LeptonInjector/serialization.cxx`.
  Like with the hdf5 serializer, no mechanism exists to write `MinimalInjectionConfiguration`
  objects. This file also contains a function to write boost-serializable
  data to plain binary files. No provisions for reading data exist (although
  I assume this would need to exist within LeptonWeighter).
  I suggest adding documentation on what serialization scheme is to be used
  in production and why.
  
* If there is no need to ever serialize `MinimalInjectionConfiguration` I would
  suggest adding documentation describing why it is not necessary.

* The main code in `private/LeptonInjector/LeptonInjector.cxx` looks reasonable
  except for some minor issues that SHOULD be fixed before production:
  - line 907: LeptonInjectorBase objects are created as bare pointers but no
    further memory management is performed. This will leak memory since objects
    are never deleted. Trivially change to `unique_ptr` unless there is some major 
    concern with performance. Change type from `std::deque<LeptonInjectorBase*> generators`
    to `std::deque<boost::unique_ptr<LeptonInjectorBase> > generators`. Also remove
    the try/except block here. It serves no purpose once changed to shared pointers.
    Since this only happens at module configuration time (and thus very likely only
    once), it is not a severe issue, but should be fixed anyway.

* Python code: `LeptonInjector/python/__init__.py` contains exclusively commented
  code. Either re-instate the code or remove it from `__init__.py` (potentially
  adding a reference so that it can be re-extracted from version control history
  in case future developers need it for reference).
  

Coding standards:

* Coding standards look reasonable, logging is included and variable naming is
  reasonably clear.

Readability:

* I was mostly able to follow the code structure. Some comments on it are included
  above.


Overall suggestions:

* Add some basic documentation as soon as possible.
* There might be some issues that could arise once this collection of modules
  is used as part of a larger-scale simulation chain. Checking these has not
  been part of this review.
* Fix the minor issues listed above.
* I recommend merging LeptonInjector into combo/trunk at this point so that
  test are regularly run and development and test of a larger-scale simulation
  pipeline including LeptonInjector can continue.