Project

General

Profile

Feature #927

Cerenkov merge/Review

Added by Rogers, Chris over 11 years ago. Updated over 11 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Kafka, Gene
Category:
Ckov
Target version:
MAUS-v0.2.0
Start date:
05 March 2012
Due date:
% Done:

100%

Estimated time:
Workflow:
Closed

Description

I had a look through the Cerenkov code with Gene. Had a few comments.

So before the merge, we need unit tests. Unit tests check that you didn't make any typos, and can be run by anyone (so everyone can check that your code works). We need to have a new script called

src/map/MapPyCkov/TestMapPyCkov.py

done in the same format as other similar things e.g. src/map/MapPyBeamMaker. The idea here is to check that the code does what you think i.e. their are no logic errors, no typos, etc

Also every function needs a docstring like

def my_function(arg_1, arg_2):
    """ 
    <short description>

    <long description, definition of args, definition of expected types, explanation of return values>
    """ 
    <function call>

and it needs to pass the python style guide, called by

python tests/style/test_python_style.py

At some point we would also like to add some scripts that check that the code really reconstructs some sample data in a correct way. So for example, light yield distributions are correct etc.

For more details, see the following documentation
Coding_style
Unit_tests
Application_tests

#1

Updated by Rogers, Chris over 11 years ago

Okay, I'm doing the merge now. Things which needed changing:

======================================================================
FAIL: Test reading the whole file
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cr67/G4MICE/MAUS/maus_merge/build/", line 84, in test_multi
    '426cc54172fc1091185f4eaacdd8b85a')
AssertionError: 'e9d641d5b2bee3298b96e84feca4f608' != '426cc54172fc1091185f4eaacdd8b85a'

This test just watches for the DAQ output changing using an md5sum. So we simply need to change line 84 of src/input/InputCppDAQOfflineData/test_InputCppDAQOfflineData.py from

         self.assertEqual(digester.hexdigest(), \
                          '426cc54172fc1091185f4eaacdd8b85a')

to
         self.assertEqual(digester.hexdigest(), \
                          '0436e0897ad0ebfa901fb9d07e28a4a1')

Also, I got a test fail because the configuration data didn't seem to have gone into ConfigurationDefaults.py causing MapPyCkov.py to fail to birth.

======================================================================
FAIL: test_init (__main__.MapPyCkovTestCase)
Check that birth works properly
----------------------------------------------------------------------
Traceback (most recent call last):
  File "src/map/MapPyCkov/test_MapPyCkov.py", line 45, in test_init
    self.assertTrue(success)
AssertionError: False is not true

I added lines 245-248:

#cerenkov calibration
ckov_position_threshold = 40
ckov_pulse_area_threshold = 1
ckov_window_min = 19
ckov_window_max = 80

Note that this passes on the test server, but I couldn't see how! Anyway, your tests saved me committing broken code, so well done.

#2

Updated by Rogers, Chris over 11 years ago

Humm, looks like the inputCppDAQData bug is more serious - your pulse area algorithm is unstable...

#3

Updated by Rogers, Chris over 11 years ago

as in it produces different answer between different executions of the code.

#4

Updated by Rogers, Chris over 11 years ago

Aha - found it:

int fADCDataProcessor::get_neg_signal_area(int&pos) {
  vector<int>::iterator it = _data.begin();
  vector<int>::iterator min;
  int area = 0;

  min = min_element(_data.begin(), _data.end());
  pos = distance(_data.begin(), min);

  if (pos > 10) {
    it = min -  10;
    while (it < min + 20) {
      area += abs(*it - _pedestal);
      it++;
    }
  }
  return area;
}

Added Yordan as a watcher because it was his test that caught the bug. Excercise for reader to spot the error in this code snippet - _data is a vector<int> and _pedestal is an int. Hints (and solution) below. Rats redmine ruins my spoiler space

-------------- spoiler space -------------
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
HINT 1: its a buffer overrun
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.

HINT 2: it's in the while statement

.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
SOLUTION: 

while (it < min + 20 && it < _data.end()) {

If _data length is less than min+20 we run off the end of the buffer

#5

Updated by Rogers, Chris over 11 years ago

Okay, the other problem is the ReducePyCkov test has poor coverage - output from tests/unit_tests.bash:

Name                                  Stmts   Miss  Cover   Missing
-------------------------------------------------------------------
ReducePyCkov                            229    115    50%   62, 69-70, 76-85, 93-185, 189-213, 325-332, 340-398

so this means 229 lines of code, 115 lines not tested => 50 % test coverage. Missing lines are then listed. I will let this code into the trunk, this time, and raise a bug on you Gene to fix it.

#6

Updated by Rogers, Chris over 11 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100
  • Workflow changed from New Issue to Closed

And committed to trunk

#7

Updated by Rogers, Chris over 11 years ago

  • Target version changed from Future MAUS release to MAUS-v0.2.0

Also available in: Atom PDF