Skip to content

Fixed of FDD digit2raw and digit writer/reader#5617

Merged
shahor02 merged 9 commits into
AliceO2Group:devfrom
mbroz84:dev
Mar 15, 2021
Merged

Fixed of FDD digit2raw and digit writer/reader#5617
shahor02 merged 9 commits into
AliceO2Group:devfrom
mbroz84:dev

Conversation

@mbroz84
Copy link
Copy Markdown
Contributor

@mbroz84 mbroz84 commented Mar 4, 2021

  • Fix of warnings seen when reading raw data made from MC digits
  • DigitWriterSpec moved from Steer to FDDWorkflow, other obsolete DigitWriter deleted
  • DigitReader reading Trigger Inputs branch

AllaMaevskaya
AllaMaevskaya previously approved these changes Mar 4, 2021
View reviewed changes
shahor02
shahor02 requested changes Mar 6, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mbroz84
thanks for fixes, but there are still some problems:

  1. when reading / writing digits related to raw data, the MC (and triggerInputs) must be disabled.
    I've fixed this and opened a PR in your repo, please merge it and forcepush).

  2. testing on 200 pbpb events generated as

o2-sim-digitizer-workflow
o2-fdd-digit2raw  --configKeyValues "HBFUtils.nHBFPerTF=128;HBFUtils.orbitFirst=0" -o raw/FDD

o2-raw-file-reader-workflow --input-conf raw/FDD/FDDraw.cfg | o2-fdd-flp-dpl-workflow

I see differences between the digits from simulation and decoding, e.g. (decoded is red one: there is apparently an overflow leading to a truncation in the amplitude, while nChanA looks like not filled but differing due to different initialization in simulation and decoding (-1 / 127).
fdd

  1. converting the same digits to raw with 16 orbits per TF (and obtaining the same data in 3 TFs), and running
o2-raw-file-reader-workflow --input-conf raw16/FDD/FDDraw.cfg --configKeyValues "HBFUtils.nHBFPerTF=16;HBFUtils.orbitFirst=0" | o2-fdd-flp-dpl-workflow

I see difference (this one is red) wrt the raw data from the previous exercise:
fdd_manytf

Apart from that, I see only 1 entry in the output digits tree, while there should have been 3 (==nTF)

I've uploaded the data you need to reproduce this here:

tar cvzf fdd.tar.gz fdddigits.root o2sim_g*.root raw16/FDD raw/FDD
fdddigits.root
o2sim_geometry.root
o2sim_grp.root
raw16/FDD/
raw16/FDD/fdd.raw
raw16/FDD/FDDraw.cfg
raw/FDD/
raw/FDD/fdd.raw
raw/FDD/FDDraw.cfg

Could you please fix this and make sure that the output of both single- and multi-TF reconstruction agrees with simulation.

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Mar 6, 2021

ps: it may happen that having just 1 entry in the output digits tree in case of multiple TF decoding is due to the problem in RootTreeWriter, I see it also in some other detectors. I will check this, meanwhile, please check the p.2.

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Mar 6, 2021

@mbroz84 your digitWriter was not storing more than 1 TF because it was initialized with request to close the tree after 1st event. I've fixed this in the PR open in your repository. So, only the problem if different content of simulated and decoded digits remain.

@mbroz84
Merge pull request #2 from shahor02/fdd_fixMC
18fd3e9 
disable MC in DigitWriter/Reader when used with raw data
@mbroz84
Copy link
Copy Markdown
Contributor Author

mbroz84 commented Mar 8, 2021

The inconsistencies in digits after Digit2Raw + Raw2Digit should be fixed now.

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Mar 8, 2021

Hi @mbroz84 thanks, but the CI produces this error:

/mnt/mesos/sandbox/sandbox/o2/sw/SOURCES/O2/5617/0/Detectors/FIT/FDD/reconstruction/src/ReadRaw.cxx:84:23: error: narrowing conversion of '(& lut)->o2::fdd::LookUpTable::getChannel(link, ((int)((long unsigned int)eventData[(2 * i)].o2::fdd::EventData::<anonymous>.o2::fdd::EventData::<unnamed union>::<anonymous>.o2::fdd::EventData::<unnamed union>::<unnamed struct>::channelID)))' from 'int' to 'uint8_t {aka unsigned char}' inside { } [-Werror=narrowing]
             chData = {int(lut.getChannel(link, int(eventData[2 * i].channelID))),
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mnt/mesos/sandbox/sandbox/o2/sw/SOURCES/O2/5617/0/Detectors/FIT/FDD/reconstruction/src/ReadRaw.cxx:99:23: error: narrowing conversion of '(& lut)->o2::fdd::LookUpTable::getChannel(link, ((int)((long unsigned int)eventData[((2 * i) + 1)].o2::fdd::EventData::<anonymous>.o2::fdd::EventData::<unnamed union>::<anonymous>.o2::fdd::EventData::<unnamed union>::<unnamed struct>::channelID)))' from 'int' to 'uint8_t {aka unsigned char}' inside { } [-Werror=narrowing]
             chData = {int(lut.getChannel(link, (eventData[2 * i + 1].channelID))),
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@shahor02
Copy link
Copy Markdown
Collaborator

shahor02 commented Mar 15, 2021

Hi [~mbroz]

I've run with your PR over the data from my previous message, the errors are gone but in both (re-)generated and decoded digits the nChanA/C, nTimeA/C and amplA/ are empty. Could you check please?

I prefer to merge this to avoid errors in the full system test, the fix can be added in other PR

@shahor02 shahor02 merged commit 0748bbb into AliceO2Group:dev Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shahor02 shahor02 shahor02 approved these changes

@jotwinow jotwinow Awaiting requested review from jotwinow jotwinow is a code owner

@mslupeck mslupeck Awaiting requested review from mslupeck

@sawenzel sawenzel Awaiting requested review from sawenzel sawenzel is a code owner

+1 more reviewer

@AllaMaevskaya AllaMaevskaya AllaMaevskaya left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mbroz84 @shahor02 @AllaMaevskaya