FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed of FDD digit2raw and digit writer/reader by mbroz84 · Pull Request #5617 · AliceO2Group/AliceO2 · GitHub

/ AliceO2 Public

Fixed of FDD digit2raw and digit writer/reader - #5617

Merged
shahor02 merged 9 commits into
AliceO2Group:devfrom
mbroz84:dev
Mar 15, 2021
ConversationCommits (9)ChecksFiles changed
Merged

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

Conversation

mbroz84 commented Mar 4, 2021

Copy link
Copy Markdown
Contributor
  • 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 previously approved these changes Mar 4, 2021
View reviewed changes
shahor02 requested changes Mar 6, 2021
View reviewed changes

shahor02 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

  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:

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 commented Mar 6, 2021

Copy link
Copy Markdown
Collaborator

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 commented Mar 6, 2021

Copy link
Copy Markdown
Collaborator

@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.

Merge pull request #2 from shahor02/fdd_fixMC
18fd3e9 
disable MC in DigitWriter/Reader when used with raw data

mbroz84 commented Mar 8, 2021

Copy link
Copy Markdown
Contributor Author

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

shahor02 commented Mar 8, 2021

Copy link
Copy Markdown
Collaborator

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 commented Mar 15, 2021

Copy link
Copy Markdown
Collaborator

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 merged commit 0748bbb into AliceO2Group:dev Mar 15, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

shahor02 shahor02 approved these changes
jotwinow Awaiting requested review from jotwinow jotwinow is a code owner
mslupeck Awaiting requested review from mslupeck
sawenzel Awaiting requested review from sawenzel sawenzel is a code owner
+1 more reviewer
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

Back | FazBrowse Home | New Git URL