[code review] Internal Review of Rock Bottom Framework

[Emily Dvorak]

Basic Code review for the new scripts in Lilliput and the whole Rock Bottom Framework. Currently located here ​https://code.icecube.wisc.edu/projects/icecube/browser/IceCube/sandbox/edvorak/Review?rev=166854&order=name

[Alex Olivas]

This likely won't be included in the next release, but I would like the review to either done or well underway by the next milestone.

The plan is to find another couple volunteers to help me with the review and we divide and conquer.

[Alex Olivas]

I think the three of us as reviewers should be sufficient. We'll figure out today how best to carve this up.

[Emily Dvorak]

committed comments and edited docs/index.rst to lilliput in my sandbox

[Kai Krings]

Hi, I've started working on the code review. The first thing that needs to be fixed: the project does not compile in debug mode...

Scanning dependencies of target rock_bottom
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/I3ParameterMap.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/I3TopRockBottomUtilities.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/interface/I3CurveLikelihood.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/interface/I3TopLDFLikelihood.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/interface/I3TopSignalModel.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/models/I3GaussCurveModel.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/models/I3LaputopSignalModel.cxx.o
[ 24%] Building CXX object rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/models/I3TwoLDFSignalModel.cxx.o
In file included from /scratch/kkrings/combo/trunk/src/icetray/public/icetray/IcetrayFwd.h:38:0,
                 from /scratch/kkrings/combo/trunk/src/dataclasses/public/dataclasses/Utility.h:29,
                 from /scratch/kkrings/combo/trunk/src/dataclasses/public/dataclasses/I3Position.h:22,
                 from /scratch/kkrings/combo/trunk/src/dataclasses/public/dataclasses/physics/I3Particle.h:12,
                 from /scratch/kkrings/combo/trunk/src/rock_bottom/public/rock_bottom/models/I3TwoLDFSignalModel.h:23,
                 from /scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:20:
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx: In member function ‘virtual double I3TwoLDFSignalModel::GetSignalMean(const I3TankGeo&, const I3VEMCalibration&, const I3Position&) const’:
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:532:13: error: ‘x’ was not declared in this scope
             x, y, z,
             ^
/scratch/kkrings/combo/trunk/src/icetray/public/icetray/I3Logging.h:100:32: note: in definition of macro ‘I3_HIFREQ_LOGGER’
     I3LoggingStringF(format, ##__VA_ARGS__)); \
                                ^
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:529:3: note: in expansion of macro ‘log_trace’
   log_trace("core (%f, %f, %f), pos (%f, %f, %f) -> (%f, %f, %f) -> %f",
   ^
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:532:16: error: ‘y’ was not declared in this scope
             x, y, z,
                ^
/scratch/kkrings/combo/trunk/src/icetray/public/icetray/I3Logging.h:100:32: note: in definition of macro ‘I3_HIFREQ_LOGGER’
     I3LoggingStringF(format, ##__VA_ARGS__)); \
                                ^
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:529:3: note: in expansion of macro ‘log_trace’
   log_trace("core (%f, %f, %f), pos (%f, %f, %f) -> (%f, %f, %f) -> %f",
   ^
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:532:19: error: ‘z’ was not declared in this scope
             x, y, z,
                   ^
/scratch/kkrings/combo/trunk/src/icetray/public/icetray/I3Logging.h:100:32: note: in definition of macro ‘I3_HIFREQ_LOGGER’
     I3LoggingStringF(format, ##__VA_ARGS__)); \
                                ^
/scratch/kkrings/combo/trunk/src/rock_bottom/private/rock_bottom/models/I3TwoLDFSignalModel.cxx:529:3: note: in expansion of macro ‘log_trace’
   log_trace("core (%f, %f, %f), pos (%f, %f, %f) -> (%f, %f, %f) -> %f",
   ^
make[2]: *** [rock_bottom/CMakeFiles/rock_bottom.dir/private/rock_bottom/models/I3TwoLDFSignalModel.cxx.o] Error 1
make[1]: *** [rock_bottom/CMakeFiles/rock_bottom.dir/all] Error 2
make: *** [all] Error 2

[Kai Krings]

More things I've noticed so far:

• there are no unit tests,
• there is only little documentation,
• there are executable Python scripts in the python directory,
• IgnoreFrame.py and WriteSeed.py contain the same i3-modules,
• most tray segments have no doc strings,
• the indentation and the usage of spaces versus tabs is not consistent.

[Alex Olivas]

The review is officially underway. There's no chance this'll be done this week and the plan is to review lilliput and rock bottom separately, so I'm pushing the milestone.

The lilliput review should be done by the Spring release. The rock bottom part might take longer, depending on how long it takes to round out the framework, and include tests and documentation. We'll see where we're at in the Spring.

Adding the other Rock Bottom authors (Javier and Dennis) to this ticket.

The plan for this is to use this ticket to track progress on the on-going development of Rock Bottom and review as we go.

[Alex Olivas]

I'll make time for this after the release comes out and before the Madison meeting.

[Alex Olivas]

This has been moved to smartsheets.

[Alex Olivas]

Milestone Vernal Equinox 2020 deleted