Support #592
Code Review InputRealData
100%
Description
In Yordan's branch
Related issues
Updated by Tunnell, Christopher over 12 years ago
What timescale are you trying to get this in the trunk? Maybe you'll do the first iteration but I'd like to take a look too before it becomes official
Updated by Tunnell, Christopher over 12 years ago
Updated by Rogers, Chris over 12 years ago
Good idea for Tunnell to have a look (better knowledge of DAQ and recon than me). I want to review the old TOF code for Beamline papers first. But I think this week my job is to have a go at a few code review trackers...
Updated by Rogers, Chris over 12 years ago
- Assignee changed from Rogers, Chris to Karadzhov, Yordan
Am I doing something wrong? Build fails with
g++ -o src/input/InputCppRealData/build/InputCppRealData.os -c -Dlong32='int' "-Dlong64='long long'" -m64 -pthread -rdynamic -fPIC -Ithird_party/install/include -Ithird_party/install/include/python2.7 -Ithird_party/install/include/root -Isrc/legacy -Isrc/common_cpp -Ithird_party/build/root_v5.28.00d/include -Ithird_party/build/geant4.9.2.p04/include -I. -Isrc/input/InputCppRealData src/input/InputCppRealData/InputCppRealData.cc In file included from src/input/InputCppRealData/InputCppRealData.cc:18: ./src/input/InputCppRealData/InputCppRealData.hh:25:27: error: MDfileManager.h: No such file or directory ./src/input/InputCppRealData/InputCppRealData.hh:26:30: error: MDprocessManager.h: No such file or directory In file included from ./src/input/InputCppRealData/InputCppRealData.hh:31, from src/input/InputCppRealData/InputCppRealData.cc:18: ./src/input/InputCppRealData/UnpackEventLib.hh:29:25: error: MDprocessor.h: No such file or directory ./src/input/InputCppRealData/UnpackEventLib.hh:34:30: error: MDfragmentVLSB_C.h: No such file or directory ./src/input/InputCppRealData/UnpackEventLib.hh:35:28: error: MDfragmentV830.h: No such file or directory ./src/input/InputCppRealData/UnpackEventLib.hh:36:27: error: MDfragmentDBB.h: No such file or directory In file included from ./src/input/InputCppRealData/InputCppRealData.hh:31, from src/input/InputCppRealData/InputCppRealData.cc:18: ./src/input/InputCppRealData/UnpackEventLib.hh:49: error: expected class-name before '{' token In file included from src/input/InputCppRealData/InputCppRealData.cc:18: ./src/input/InputCppRealData/InputCppRealData.hh:52: error: 'MDprocessManager' does not name a type ./src/input/InputCppRealData/InputCppRealData.hh:55: error: 'MDfileManager' does not name a type ./src/input/InputCppRealData/InputCppRealData.hh: In member function 'void InputCppRealData::disableEquipment(std::string)': ./src/input/InputCppRealData/InputCppRealData.hh:152: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc: In member function 'bool InputCppRealData::birth(std::string)': src/input/InputCppRealData/InputCppRealData.cc:37: error: '_dataFileManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:42: error: '_dataFileManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:71: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:85: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:100: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:111: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:122: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:133: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc: In member function 'bool InputCppRealData::readNextEvent()': src/input/InputCppRealData/InputCppRealData.cc:151: error: '_dataFileManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc: In member function 'std::string InputCppRealData::getCurEvent()': src/input/InputCppRealData/InputCppRealData.cc:185: error: '_dataProcessManager' was not declared in this scope src/input/InputCppRealData/InputCppRealData.cc:202: error: '_dataProcessManager' was not declared in this scope scons: *** [src/input/InputCppRealData/build/InputCppRealData.os] Error 1 scons: building terminated because of errors.
I guess the wrong unpacking version? Can't really do much if I can't build the code. I notice the unpacking build script in Yordan's dev area has not been changed.
Bounce back at Yordan for help...
Updated by Karadzhov, Yordan over 12 years ago
Yes, you need the new unpacking in order to run the new InputRealData.
To get the new unpacking do
bzr co lp:unpacking
Updated by Rogers, Chris over 12 years ago
- Assignee changed from Karadzhov, Yordan to Rogers, Chris
Thanks
Updated by Rogers, Chris over 12 years ago
Cut n paste from #550
From Rogers:
Multiple inheritance in e.g V1371
Hard coded pedestal (should be datacard)
Running over socket requires recompile (to avoid sprawling dependency tree)
Try pushing up to Unpacker
More comments
Run style guide
Updated by Rogers, Chris over 12 years ago
Files to look at:
src/input/InputCppRealData.hh
src/input/InputCppRealData.cc
src/input/test_InputCppRealData.py
src/input/UnpackEventLib.hh
src/input/UnpackEventLib.cc
src/Utils/DAQChannelMap.hh
src/Utils/DAQChannelMap.cc
I think not (I guess this is Tof Recon stuff):
src/Utils/TOFChannelMap.*
Updated by Rogers, Chris over 12 years ago
General¶
In general, I like the code, I think it looks pretty robust and nicely polished. I have a few criticisms, maybe also some bugs (or things that look like bugs to me), but that's my job. Apologies.
Unpacking library¶
- Unpacking library should be installed by a bash script, like all the other third party things. Need to make a "good" version of unpacking and put either in MAUS files area or somewhere on web.
- Unpacking needs to build a .so file - requires some hacking in the unpacking makefile to get that to work.
- Would be good to put unpacking library into install/include/unpacking so that includes look like #include "unpacking/MDpartEventV1731.h" or whatever. Just makes it a bit more obvious what is MAUS and what is not.
Doxygen / comments¶
- Good doxygen coverage. Nice.
- Say InputCppRealData is a prototype "RealData" module. No longer prototype I hope!
- V1724 + V1731 brief summary doesn't come through
- Few functions undocumented - could do with one liners; DAQChannelKey functions, zero suppression fADCDataProcessor::get_pedestal_area, fADCDataProcessor::set_zero_suppression, fADCDataProcessor::set_zs_threshold, MDarranger::get/set_json_doc, V1731DataProcessor::set_zero_supression
Code¶
General¶
The code is of a good standard and robust. Of course, I have to go through and make some detailed comments, but in general it's really nice.
InputCppRealData¶
- General
- Style - public then private; colon on same line - like
public: void Foo(); private: void Bah();
- Style - why map instead of _map? (underscore missing) No big deal of course...
- Style - public then private; colon on same line - like
- birth()
- Style - Better to raise a Squeal exception on error instead of calling exit
- Bug - getenv("MAUS_ROOT_DIR") returns NULL if variable not found => segmentation fault
- Style - don't do namespace::std please
- Style - Maybe better to do datacard like "V1724_zero_suppression_threshold" rather than "V1724_zero_suppressio_threshold" (missing n). Same for all these in fact
- Style - Please use Squeak::mout(Squeak::debug) rather than std::cout; and Squeak::mout(Squeak::error) rather than std::cerr. At some point I want to implement logging output for control room running. Please don't define your own _debug variable, use the VerboseLevel configuration datacard.
- Style - why not use inheritance to reduce "cut and paste"?
UnpackEventLib¶
- Everywhere you use "pedestals" as the data tree name, did you mean to write "pedestal"?
- You use -99 as an error code. Fine, maybe you could soft code it?
- I didn't understand what the logic in the for loop was doing. My lack of knowledge about DAQ for sure, but perhaps means a comment here would help out?
- Query - everywhere you assume 20 bins to be the length of an event. Did you ever check that this is okay? Would it be better to soft code this?
- set_pedestal() - did you ever worry that there might be a hit in the first 20 bins when you calculate a running pedestal?
- get_pedestal_area - what is the origin of the hard coded 97 here?
- Bug at line 167: I guess xch < V1724_NCHANNELS should be xch < V1731_NCHANNELS? Not sure where this is defined.
- would it be better to use uint32_t for the pointer at line 218 rather than an unsigned int - guarantees that it is 32 bit pointer
- is the idea that tracker group adds more logic here later on? I sort of expected this to be a bit more complicated? But maybe all the logic is in their FPGA?
- Bug at line 307: I guess V1290_NCHANNELS - should be DBB_NCHANNELS? Not sure where this is defined.
DAQChannelMap¶
- DAQChannelKey
- Style - might be better to make private data really private
- Bug? - make_DAQChannelKey_id() do you worry about hardware with more than 100 channels? e.g. tracker has > 300 (but maybe not all on same board so it's okay?)
- Style - operator should take const reference.
- Style - operator != should take const reference. (Fine just to return NOT ).
I like the way you overloaded the operators here. Makes nice, easy going code. If you get bored, can do < > operators to help with e.g. std::sort, std::find algortihms.
- DAQChannelMap
- Please don't call exit() - throw an exception
- Find algorithm is correct, but slow. If it gets used a lot, may prefer to use STL find/sort algorithms
Tests¶
You know what I'm going to say here - tests aren't thorough enough. Even worse, the test fails on my machine (test log attached).
How much checking did you do by hand? For example, did you check that you can unpack every equipment type correctly? Did you check that you can handle errors correctly? Would be great to at least have automated test that runs through every board type and checks that we can unpack every equipment type, some of the switches work, etc. I wanted to get > 80% test coverage, at the moment you have about 70%, and you don't check the output as thoroughly as I would like.
Memory leaks¶
You might be interested to know:- Valgrind reports memory leak in _dataProcessManager
- Valgrind reports memory leak in _dataFileManager.OpenFile()
Thanks a lot for contributing the code, as I said previously in general it looks really good - just tidying mostly, although getting the tests done might take a bit of effort.
Updated by Rogers, Chris over 12 years ago
- Assignee changed from Rogers, Chris to Karadzhov, Yordan
Push back to Yordan. Chris Tunnell may have comments also.
Updated by Tunnell, Christopher over 12 years ago
I know we're back recently from Nufact so things are slow.
Yordan: are you pushing a new version? It would be nice to have this in the trunk by the next software phonecall if there are no major changes. I could have time later in the week to peek at it.
Updated by Karadzhov, Yordan over 12 years ago
Yes I did the modifications suggested by Chris Rogers, but I would like to have some time to play with the new code just to be sure that everything works properly. Also I have to investigate the memory leak problem.
This is a question to Chris Rogers: do you mean that the memory leak is in the unpacking?
Updated by Rogers, Chris over 12 years ago
I think so - valgrind can sometimes make mistakes, but that's what it looked like.
ps: My aim is that the sort of checks you do to make sure code is working properly get put into tests that we can rerun every time the code is changed. Sometimes I make a small change that shouldn't break anything, but break stuff by accident. Sometimes changes in other code breaks things in my code. Making it easy to check code stops these sorts of problems (but does take a bit of extra time to set up).
Updated by Rogers, Chris over 12 years ago
- Target version changed from Future MAUS release to MAUS-v0.1.0
Updated by Rogers, Chris about 12 years ago
I pulled down the latest version and still am getting a test fail on my machine.
self.assertEqual(digester.hexdigest(), '52a4a8021cfdef2d6e220c39c679970b')
Gives an error like
cr67> python build/test_InputCppRealData.py Creating the equipment map ... --- Equipment Map Dump --- 80 0xc2c340 VLSB_C 0xc352a0 102 0x7be560 V1290 0x7be4e0 111 0xc2da30 V830 0x7ccd00 120 0xc2e100 V1724 0xc2c280 121 0xc2e0e0 V1731 0xc2e060 141 0xc2b9b0 DBB 0xc343c0 Equipment Map count : 1 Going to search for run 02873 data files in /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData Trying to add file 02873.003 from /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData File 02873.003 is added .WARNING in MDequipMap::MDequipMap() : Trying to create a multiple instances of a static singleton class --- Equipment Map Dump --- 80 0xc2c340 VLSB_C 0xc352a0 102 0x7be560 V1290 0x7be4e0 111 0xc2da30 V830 0x7ccd00 120 0xc2e100 V1724 0xc2c280 121 0xc2e0e0 V1731 0xc2e060 141 0xc2b9b0 DBB 0xc343c0 Equipment Map count : 2 Going to search for run 02873 data files in /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData Trying to add file 02873.003 from /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData File 02873.003 is added WARNING : The first event is not a START_OF_RUN. Spill count and Event count not accurate. ++++ End of file 02873.003 ++++ FWARNING in MDequipMap::MDequipMap() : Trying to create a multiple instances of a static singleton class --- Equipment Map Dump --- 80 0xc2c340 VLSB_C 0xc352a0 102 0x7be560 V1290 0x7be4e0 111 0xc2da30 V830 0x7ccd00 120 0xc2e100 V1724 0xc2c280 121 0xc2e0e0 V1731 0xc2e060 141 0xc2b9b0 DBB 0xc343c0 Equipment Map count : 3 Going to search for run 02873 data files in /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData Trying to add file 02873.003 from /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData File 02873.003 is added WARNING : The first event is not a START_OF_RUN. Spill count and Event count not accurate. . ====================================================================== FAIL: test_multi (__main__.InputCppRealDataTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "build/test_InputCppRealData.py", line 61, in test_multi self.assertEqual(digester.hexdigest(), '52a4a8021cfdef2d6e220c39c679970b') AssertionError: 'e17c6bd815a383558761702d46e31dca' != '52a4a8021cfdef2d6e220c39c679970b' ---------------------------------------------------------------------- Ran 3 tests in 3.735s FAILED (failures=1)
Updated by Rogers, Chris about 12 years ago
I'll just check that there wasn't some problem in pulling the latest version...
Updated by Rogers, Chris about 12 years ago
Did a fresh checkout and it passes. Must have been a problem at my end... sorry about that.
Updated by Rogers, Chris about 12 years ago
I did a first merge - and now am hacking around trying to "improve" some stuff. e.g. I wanted to put unpacking in a subdirectory of include, to make it easier to manage, want the binaries (analyze_mice.py) to be driven by datacards...
Updated by Rogers, Chris about 12 years ago
So I messed around with a few things:
- unpacking is now in a subdirectory
- analyze_offline binary is driven entirely by datacards
- added small test to check analyze_offline does something (integration test)
I want additionally to move README.txt into the SpillSchema.py, but I don't understand really what the spill looks like, and running analysis looks weird
{ "spill_num": -1, "daq_event_type": "end_of_burst", "daq_data": null } { "spill_num": 0, "daq_event_type": "start_of_burst", "daq_data": null } { "spill_num": 0, "daq_event_type": "physics_event", "daq_data": { "V830": { ... { "spill_num": 0, "daq_event_type": "end_of_burst", "daq_data": null } { "spill_num": 1, "daq_event_type": "start_of_burst", "daq_data": null } { "spill_num": 1, "daq_event_type": "physics_event", "daq_data": { "V830": { ... etc
This looks wrong. We are supposed to be doing one line per spill, whereas looks like start_of_burst is split onto a separate line. Can we fix this?
Updated by Karadzhov, Yordan about 12 years ago
In DATE we have big number of different DAQ event types. For the moment the MICE DAQ fills information only in "physics_event" but this may change in the future, so I do not want the unpacking to bypass the other event types. Also keep in mind that DATE package has been made for ALICE so they use the LHC terminology. In our case "burst" is equivalent to "spill".
Updated by Rogers, Chris about 12 years ago
The data should be filled one json document per spill. At the moment, unpacking creates one json document for start_of_burst, one document for end_of_burst, one document for physics_event. These should be all in the same document, say in a "daq_data" branch of the spill document.
Updated by Tunnell, Christopher about 12 years ago
We can always add another branch if we get calibration events.
Updated by Rogers, Chris about 12 years ago
- Status changed from Open to Closed
- % Done changed from 0 to 100
I'm going to open it as a new feature request and close this issue.
Updated by Rogers, Chris about 12 years ago
- Target version changed from MAUS-v0.1.0 to MAUS-v0.0.9