Skip to content
Snippets Groups Projects

Algorithm for LAr ByteStream reading

Merged Walter Lampl requested to merge wlampl/athena:LArRCBSCnvAsConstAlgo into master

For @damazio and @pavol attention: A possible implementation of Re-entrant algorithm reading LArRawChannels for ByteStream.

Notes/Comments:

  • So far reading the entire RawEvent and using the eformat::helper::create_toc to filter out LAr-fragments. Do we have somewhere a list of all LAr ROB-ids?
  • I did not implement all the checks for data corruption we have in the old converter
  • In this simple version, the cxx is less than 150 lines long with only ~30 lines of boilerplate code that could be shared with conversion-algorithms of LAr(Calib)Digits or LArFEBHeader.

Comments are welcome!

Edited by Walter Lampl

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Walter Lampl added 3 commits

    added 3 commits

    • 84b8b5ba - remove useless/commentd code lines
    • 60c3fc99 - LArRawChannelBSReadAlg: add checksum verification, refine handling of data corruption
    • 65c247ae - rename LArRawChannelBSReadAlg into LArRawDataReadingAlg in preparation of generalizing it

    Compare with previous version

  • Walter Lampl added 2 commits

    added 2 commits

    • 1d394bd8 - LArRawDataReadingAlg: Add reading of LArDigitContainer
    • 91bda00f - LArRawDataReadingAlg: Add reading of LArFebHeader

    Compare with previous version

  • As discussed with @pavol yesterday, extended this prototype to read also Digits and the data from the headers. They are stored in the same ROD-Fragment, so it makes sense to decode them at the same time. The implementation is still less than 300 lines and I believe it's well structured. That compares to >1000 lines in the reading part of old converter that we wrote in 2003 (and extended since then).

    The actual decoding of the bits & bytes written by the readout electronics is done by the same RodBlockStructure classes that are used by the old converter I did not dare to touch them.

    @damazio : If you stick to the service-approach for data-reading in the HLT, I think the RodBlockStructure classes are the only piece of code worth being shared between HLT and offline. The boilerplate code around is only some 10 lines.

  • Hello Walter,

    I think I will need a bit more time to check all this. That said, I think we should not decode the digits with the energies. This can slow down things. Can we keep the LArRodDecoder instead?! I will try to read everything in the next two or three days. Please, remember that I also have to deal with the Tile, which has a similar interface (fillCollectionsHLT) which I would prefer to keep (see that I DID NOT read yet the code..) Thanks, Denis

  • Hi @damazio,

    the decoding of RawChannels / Digits / FebHeaders is optional. One can turn each of them off by jobO.

    For the trigger code, do you actually need the LArRawChannel object? I thought you are bypassing this step. I am pretty sure you'll need only a handful of code-lines from the LArRodDecoder (which is an overloaded and clumsy class)

    • Walter
  • We don't need the LArRawChannel object. That is why we have fillCollectionHLT with LArCellCollection.. Denis

  • Walter Lampl added 3 commits

    added 3 commits

    • 27a569f5 - cosmetic changes (comments, messages, etc.)
    • 91671855 - jobOption changes so that RecExCommon reads LAr raw data via...
    • 15a48a93 - Update LArRawChannelBuilderAlgConfig to use LArRawDataReadingAlg (instead of converter)

    Compare with previous version

  • With the changes above, the q431 test shows:

    • No reference to LAr-data any more in ByteStreamAddressProviderSvc.TypeNames
    • The LArRodDecoder public AlgTool is still in the job, probably because of TrigDataAccess service.
    • The resulting ESD is identical

    Up-wip'ing now.

    • Walter
  • Walter Lampl unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Can you wait until tomorrow morning before merging? I’d like really to be able to look and understand it.. Thanks Denis

  • This merge request affects 5 packages:

    • Calorimeter/CaloRec
    • Event/ByteStreamCnvSvc
    • LArCalorimeter/LArCnv/LArByteStream
    • LArCalorimeter/LArROD
    • Reconstruction/RecExample/RecExCommon

    Adding @pavol as watcher

  • Walter Lampl added 1 commit

    added 1 commit

    • 89dfb6ed - LArRawDataReadingAlg: Reseve vectors before filling

    Compare with previous version

  • This merge request affects 5 packages:

    • Calorimeter/CaloRec
    • Event/ByteStreamCnvSvc
    • LArCalorimeter/LArCnv/LArByteStream
    • LArCalorimeter/LArROD
    • Reconstruction/RecExample/RecExCommon

    Adding @pavol as watcher

  • Hi @damazio,

    I havn't touched any trigger code and havn't touched the old converter code.

    • Walter
  • :negative_squared_cross_mark: CI Result FAILURE

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    required tests :o: :white_check_mark:
    optional tests :white_check_mark: :white_check_mark:

    Full details available at NICOS MR-25404-2019-08-07-17-56
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 1896]

  • :negative_squared_cross_mark: CI Result FAILURE

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    required tests :o: :white_check_mark:
    optional tests :white_check_mark: :white_check_mark:

    Full details available at NICOS MR-25404-2019-08-07-18-10
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 1900]

  • About the failing test Trigger_athena_MC_v7: It works locally if I feeed it with only one input file. With two input files the meta-reader fails. No idea how this could be triggered by my MR.

    • Walter
  • I assume !25436 (merged) fixes the issue. Jenkins, please retry a build

  • Jenkins please retry a build

  • This merge request affects 5 packages:

    • Calorimeter/CaloRec
    • Event/ByteStreamCnvSvc
    • LArCalorimeter/LArCnv/LArByteStream
    • LArCalorimeter/LArROD
    • Reconstruction/RecExample/RecExCommon

    Adding @pavol as watcher

  • :negative_squared_cross_mark: CI Result FAILURE

    Athena AthSimulation
    externals :white_check_mark: :o:
    cmake N/A N/A
    make N/A N/A
    required tests N/A N/A
    optional tests N/A N/A

    :exclamation: Due to problems in externals build or cmake configuration the job is stopped, results are not available on NICOS Web UI
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 1919]

  • Jenkins please retry a build

  • Restarted after as Jenkins update unexpectedly affected this job

  • This merge request affects 5 packages:

    • Calorimeter/CaloRec
    • Event/ByteStreamCnvSvc
    • LArCalorimeter/LArCnv/LArByteStream
    • LArCalorimeter/LArROD
    • Reconstruction/RecExample/RecExCommon

    Adding @pavol as watcher

  • :white_check_mark: CI Result SUCCESS

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    required tests :white_check_mark: :white_check_mark:
    optional tests :white_check_mark: :white_check_mark:

    Full details available at NICOS MR-25404-2019-08-08-04-50
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 1928]

  • Hello Walter,

    I can see that only a few types are instantiated (not all the different calibration types, transparent types, etc..). Won’t this be a future headache?! All the others have been phased out or left to old releases. I would like to do something similar in the HLTCalo actually.. We could reduce the size of the LArRodDecoder itself.

    The execute method is like a large fillCollection. I fear it would be great to keep it in a LArRodDecoder, if possible unifying everything. Otherwise, code may change in the HLT and not be followed in the offline and vice-versa.. Maybe we could do that instead? What do you think? The tool would be controlled by this algorithm. Would you mind if I do that?!

    I am worried about the need of some particular conditions (cabling, first of the FEBs in a ROD payload, etc). Maybe we could by pass that in a check in the first event?!

    Do we have a simple running jobOption that I could play with this?! I would like to check the processing time of this thing.

    As for the different types and converters, I think you are doing the right thing on removing all these stupid converters. We could control it to use the different types by options as you did, of course.

    What about encoding?! I think only HLTCalo in RDO->BS mode worries about it (and transient BS).

  • Hi Denis,

    at this point, I worry only about the data types used in standard reconstruction. For the LAr(Accumulated)CalibDigitContainer I plan to write two more algorithms like this one (not urgent). That would be another ~ 300 lines of code.

    As I wrote earlier, there is so little boilerplate code that it's not worth trying to share it in a dedicated tool like LArRodDecoder. Moreover, the LArRodDecoder has specialized fillCollection methods for each data type. So there is actually no common code, it's just collected in one tool instead of two or three algorithms. The only thing worth sharing is the LArRodBlockStructure. Apart of that, there is really nothing that could diverge between offline and HLT.

    There are (new-style) jobO in the python directory. My tests yesteday indicate that the decoding time is about 4ms/event. Vtunes suggests the bulk of that is indeed the the new'ing of LArRawChannels.

    Writing bytestream is a different story. The MR here does not address this.

    • Walter
  • would you mind if I try after this?! Can you tell me which jobOption. Also, I need a fast course on how to deal with this new configuration. You probably know of many. Could I bug you to point one out to me?! I think we can do a bit better than this (even though, this is great). I do believe people will soon complain about the other types of data.. Cheers, Denis

  • To run the new jobO (via a pickle file)

    python LArRawDataReadingConfig.py

    athena LArRawDataReading.pkl

    The only missing data types are the ones used for the electronic calibration. We'll should move this code to new-style athena as well but - as I said already - this is not a priority.

    • Walter
    Edited by Walter Lampl
  • Passing this to @tadej for a L2 review. However I think @damazio wanted to check some things before this gets merged if the comments from above are still valid, so we shouldn't change the label to review-approved yet.

  • Hello Catrin, I am fine with the changes (I will try some other changes on top of them later on). I was trying to put this comment in gitlab, but it was not working... Cheers, Denis

  • Walter Lampl mentioned in merge request !25730 (merged)

    mentioned in merge request !25730 (merged)

  • Tadej Novak
  • Tadej Novak
  • Tadej Novak
  • Tadej Novak
  • Tadej Novak
  • Hi @wlampl,

    I just have a few minor comments.

    Tadej (L2)

  • Walter Lampl added 1 commit

    added 1 commit

    • f7a1acaa - Implement review comments (copyright, ...) + fix flake8 complain

    Compare with previous version

  • Walter Lampl resolved all discussions

    resolved all discussions

  • This merge request affects 5 packages:

    • Calorimeter/CaloRec
    • Event/ByteStreamCnvSvc
    • LArCalorimeter/LArCnv/LArByteStream
    • LArCalorimeter/LArROD
    • Reconstruction/RecExample/RecExCommon

    Adding @pavol as watcher

  • :negative_squared_cross_mark: CI Result FAILURE

    Athena AthSimulation
    externals :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark:
    required tests :o: :white_check_mark:
    optional tests :white_check_mark: :white_check_mark:

    Full details available at NICOS MR-25404-2019-08-10-17-17
    :o: Athena: number of compilation errors 1, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 2061]

  • This is a bit weird, there is so much DEBUG output here: http://atlas-computing.web.cern.ch/atlas-computing/links/distDirectory/ci/CIWebArea/nicos_web_areaMRCIbuilds64BC7G8AthenaOpt/NICOS_TestLog_MR-25404-2019-08-10-17-17/ReleaseTests___Trigger_MT_required-test__Trigger_MT_required-test__m.html and it ultimately fails with Athena CRITICAL stopped by user interrupt terminate called without an active exception I wonder if this was turned on by mistake? Tagging @hartj and @jpanduro in case they know of anything or have seen this elsewhere?

  • Hi @bernius , Looks like the test timed out for some reason. Let's retry. Julie

  • Jenkins please retry a build

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading