Monopole-Generator Code Review

Sebastian Fiedlschuster, 2020-02-29

https://github.com/fiedl/monopole-generator/issues/1

Version

This review refers to the monopole-generator project in the following version: - https://github.com/fiedl/monopole-generator/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6 - http://code.icecube.wisc.edu/svn/projects/monopole-generator/trunk@173007

Compiled against: https://github.com/IceCube-SPNO/IceTrayCombo/tree/029c4839e44d8694f992bae92e839a8b288a6b90

Summary

Monopole Generator is a fairly small project with a good-to-understand purpose and implementation. Its overall structure is clear and well-designed. However, the impression was that the project has been modified several times, but lacked the necessary maintenance work during that time. Documentation and tests have been out of date and did not match the current implementation and interface anymore.

After the initial review, I’ve fixed the tests and revised the documentation. I’ve created CI scripts to build the project against combo and to run the tests. I’ve prepared the merge of the monopole-generator project into IceTrayCombo and listed recommendations for future code improvements.

As I’m not familiar with topic itself, yet, I’ve only reviewed the code, not mathematics or physics of the implementation.

About Code Reviews

General information about code reviews in icecube: http://software.icecube.wisc.edu/documentation/general/code_reviews.html

Criteria Checklist

First Glance

Result: Documentation does exist in the `resources/docs <https://github.com/fiedl/monopole-generator/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/resources/docs>`__ directory.

The wiki contains similar information, which is not in sync, however, with the `resources/docs <https://github.com/fiedl/monopole-generator/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/resources/docs>`__ directory.

Also, a README.md in the project root is not present.

Result: There is one example script in `resources/examples <https://github.com/fiedl/monopole-generator/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/resources/examples>`__. However, this script demos only one of the two modules provided by this project.

  • [ ] It would be nice to have a demo script that generates and propagates monopoles and their secondaries and displays these events in steamshovel.

  • [x] Do unit tests exist?

Result: There are python tests in `resources/test <https://github.com/fiedl/monopole-generator/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/resources/test>`__ and cxx tests in `private/test <https://github.com/fiedl/monopole-generator/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/private/test>`__, which is in agreement with the review criteria (see section Directory structure below).

  • [x] Does the code compile without warnings in both debug and release mode?

Result: Yes

  • [x] Do the tests compile without warnings?

Result: Yes

  • [x] Do all tests succeed?

Results: No.

Test coverage: The overall line coverage is 83%. These parts of the code are not covered:

  • [ ] the “correct decay” mechanism, which simulates pi particles in addition to positron cascades

  • [ ] I3MonopoleRelativisticUtils::UpdateMPInfoDict

  • [ ] I3MonopoleRelativisticUtils::MPSpeedProfile

  • [ ] a couple of sanity-check fatals

Documentation

  • [x] Does it explain in reasonable detail what the project does?

Result: Yes

  • [x] Is it understandable for non-experts?

Result: Yes

  • [x] Look at the output of icetray-inspect. Are the docstrings useful for a person reasonably acquainted with the purpose of the project? Is the usage clear?

Result: No

Further remarks on documentation:

  • The documentation in resources/docs, in the wiki, and the actual code base were not in sync. Parts of the documentation were outdated.

  • [x] Fixed in: https://github.com/fiedl/monopole-generator/issues/6

  • There exist several wiki pages describing older versions of the monopole generator.

  • [ ] We should add a comment to the top of those pages referring to the new monopole-generator documentation.

Directory Structure

The code should be organized according to http://software.icecube.wisc.edu/documentation/general/code_reviews.html#directory-structure:

Result: ok.

Result: ok

  • [x] private/pybindings - This is the directory for the python bindings.

Result: This directory does not exist. (For an impression, check clsim’s ``private/pybindings` directory <https://github.com/claudiok/clsim/tree/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/private/pybindings>`__.)

Result: ok.

Result: ok.

Result: ok.

Result: ok. The scripts call python unit tests without command line parameters.

Result: ok. The directory contains one script: `PlotGeneratingDistributions.py <https://github.com/fiedl/monopole-generator/blob/f68f19ff7a4d8fead45b4d6ae345723da69edbf6/resources/examples/PlotGeneratingDistributions.py>`__. This script is not commented. But one can find an example on how to call the monopole-generator module there.

Result: The directory contains rst and dox files.

Remarks:

Further remarks:

Code Structure

  • [x] Every function should fulfill one specific purpose.

Result: No. Several functions may be split up in shorter functions with clear names and purposes. But the code is understandable and for a project of this small size, it’s ok.

  • [x] Dead code, blocks of code that are commented out or disabled by #if 0 … #endif blocks should be removed.

Result: No. There are several blocks of dead code.

  • [x] Implementations go into .cxx files, not headers.

Result: ok.

  • [x] Header files that are not part of the module interface (e.g. classes/functions used internally by the module) go into the private directory.

Result: ok.

Coding Standards

  • [x] Consistent naming of variables, classes.

Result: No.

  • [ ] The naming of variables needs to be improved.

  • [ ] Also, a consistent case convention for variable and method names should be applied: helloworld vs. helloWorld vs. hello_world.

  • [x] variables should have meaningful names, but normal IceCube abbreviations like DOM are okay.

Result: No. (But as the code base is not very long, it’s not too harmful. However, improving the variable names would serve readability and save newcomers time when reading the code.)

Readability

  • [x] Can you follow the logic of the code?

Result: Yes.

  • [x] Code duplication should be avoided

Result: ok.

  • [x] Are error and warning messages understandable?

Result: Improvement recommended.

Some error messages (like “out of range”) do not help to fix the issue without reading the code. This can be improved by providing more constructive and informative outputs.

Usability

  • [x] Are parameters documented and understandable?

Result: No.

The documentation of the parameters has not been up to date. Also, the descriptions listed in icetray-inspect did not match the descriptions from the documentation, and tended to be not very helpful without reading the documentation and the code.

Improved in: https://github.com/fiedl/monopole-generator/issues/6

  • [x] Are parameters useful?

Result: Yes.

Detailed Annotation

I have annotated the code in its reviewed state and listed recommendations for future improvements:

https://github.com/fiedl/monopole-generator/pull/4

PDF export: 2020-02-29_code_review_appendix.pdf

Fixes After the Review

The first fixes after this review are collected in this pull request:

https://github.com/fiedl/monopole-generator/pull/7

Archiving This Code Review

This code review can be accessed on github: https://github.com/fiedl/monopole-generator/issues/1

  • [x] A copy of the review is documented in resources/docs of the monopole-generator code. (source)

  • [x] The review is linked to the project’s ReST docs (?). (source)

In order to sync this markdown file 2020-02-29_code_review.md to the equivalent rst file 2020-02-29_code_review.rst, use pandoc:

$ pandoc --from markdown --to rst resources/docs/2020-02-29_code_review.md > resources/docs/2020-02-29_code_review.rst

Next Steps