Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raw to digits #5585

Merged
merged 96 commits into from
Mar 5, 2021
Merged

Raw to digits #5585

merged 96 commits into from
Mar 5, 2021

Conversation

cortesep
Copy link
Contributor

@cortesep cortesep commented Mar 1, 2021

No description provided.

@shahor02 shahor02 changed the title WIP - Raw to digits [WIP] - Raw to digits Mar 1, 2021
@shahor02
Copy link
Collaborator

shahor02 commented Mar 1, 2021

there were also plenty of Double_t...

@shahor02
Copy link
Collaborator

shahor02 commented Mar 1, 2021

@cortesep wait, I see you have same commits in this PR and in the earlier https://github.com/AliceO2Group/AliceO2/pull/5533/files. By amending both you will introduce conflicts.

I would suggest to do

git checkout dev_zdcawrw
git rebase working_branch

Add all fixes, make sure that it compiles and works both for mc->raw and raw->digis, the forcepush by
git push -f cortesep:working_branch

Then the old one can be closed.

@cortesep
Copy link
Contributor Author

cortesep commented Mar 1, 2021 via email

@shahor02
Copy link
Collaborator

shahor02 commented Mar 1, 2021

@cortesep yes, if all the fixes (UShort_t etc) and comments are accounted in this one, just do push -f ...

@cortesep
Copy link
Contributor Author

cortesep commented Mar 1, 2021 via email

@shahor02
Copy link
Collaborator

shahor02 commented Mar 1, 2021

You don't need : in the push, just git push -f cortesep working_branch

@cortesep
Copy link
Contributor Author

cortesep commented Mar 3, 2021 via email

@shahor02
Copy link
Collaborator

shahor02 commented Mar 3, 2021

@cortesep ,
to rename the PedestalData to e.g. PedestalScalerData (or EndOfOrbitData, you decide), just do from O2 dir.:

git grep -l PedestalData | grep -i zdc | xargs perl -pi -e 's/PedestalData/PedestalScalerData/g'

Did you validate the changes on 2 PbPb data samples I put on cernbox? Is the content and the number of original and decoded digits now consistent?

@cortesep
Copy link
Contributor Author

cortesep commented Mar 3, 2021 via email

@shahor02
Copy link
Collaborator

shahor02 commented Mar 3, 2021

To hide the hit information that is known only to simulation and is not available in raw data. I didn't know how to set this through
ZDCDigitizerSpec, get the flag in its init() method, like

mDisableQED = ic.options().get<bool>("disable-qed");
and then set it to Digitizer. I would suggest to not store this info by defaults, i.e. the option should be something like

Options{{"enable-hit-info", o2::framework::VariantType::Bool, false, {"enable hit info of unread channels"}}

BTW, I would suggest also to propagate the useMC flag (--disable-mc of o2-sim-digitizer-workflow) which will allow to skip creation of labels info, see:

{"disable-qed", o2::framework::VariantType::Bool, false, {"disable QED handling"}}

Once you finish, please remove [WIP]. You could not redigitize pbpb because I did not copy Hits and Kine, they are too boog. I'll redigitize both 1 and 4 TF versions and send to you for final validation.

AlgorithmSpec{adaptFromTask<ITSDPLDigitizerTask>(mctruth)},

@cortesep cortesep requested a review from a team as a code owner March 4, 2021 06:16
@cortesep
Copy link
Contributor Author

cortesep commented Mar 4, 2021

To hide the hit information that is known only to simulation and is not available in raw data. I didn't know how to set this through
ZDCDigitizerSpec, get the flag in its init() method, like

mDisableQED = ic.options().get<bool>("disable-qed");

and then set it to Digitizer. I would suggest to not store this info by defaults, i.e. the option should be something like

Options{{"enable-hit-info", o2::framework::VariantType::Bool, false, {"enable hit info of unread channels"}}

Done.

BTW, I would suggest also to propagate the useMC flag (--disable-mc of o2-sim-digitizer-workflow) which will allow to skip creation of labels info, see:

I have an issue with disable-mc options. If I introduce it in the list of options
return DataProcessorSpec{
"ZDCDigitizer",
Inputs{InputSpec{"collisioncontext", "SIM", "COLLISIONCONTEXT", static_cast(channel), Lifetime::Timeframe}},
outputs,
AlgorithmSpec{adaptFromTask()},
Options{{"enable-hit-info", o2::framework::VariantType::Bool, false, {"enable hit info of unread channels"}},
{"disable-mc", o2::framework::VariantType::Bool, false, {"do not store MC labels"}}}
};
I have an error
Error: option '--disable-mc' is ambiguous and matches different versions of '--disable-mc'

If the option is not in the list the program crashes..

@cortesep
Copy link
Contributor Author

cortesep commented Mar 4, 2021

I used a trick to set an option coherent with mctruth argument. I don't know if this is what you meant but it seems to work

@davidrohr
Copy link
Collaborator

I think we should not use an option with a different name here (disable-mc-zdc), but it should use the same disable-mc option of all other workflows, otherwise it is misleading and failure-prone.

I checked what the TPC workflow is doing, and it defines the disable-mc option:

void customize(std::vector<o2::framework::ConfigParamSpec>& workflowOptions)
{
  using namespace o2::framework;
  std::vector<ConfigParamSpec> options{
[...]
    {"disable-mc", VariantType::Bool, false, {"disable sending of MC information"}},
[...]
  };
  std::swap(workflowOptions, options);
}

I am wondering why it should be ambiguous here. Due to the different option description perhaps?

{"disable-mc-zdc", o2::framework::VariantType::Bool, false, {"do not store ZDC MC labels"}}}

But in any case, I think we should understand and fix that.

@cortesep
Copy link
Contributor Author

cortesep commented Mar 4, 2021

Only remaining difference is that Digitizer writes a single entry for all events, while raw to digits writes one entry per TF

@cortesep cortesep changed the title [WIP] - Raw to digits Raw to digits Mar 4, 2021
@shahor02
Copy link
Collaborator

shahor02 commented Mar 4, 2021

@cortesep You don't need to add disable-mc on your digitizer device level, it is already defined on the workflow level:

workflowOptions.push_back(ConfigParamSpec{"disable-mc", o2::framework::VariantType::Bool, false, {"disable mc-truth"}});
,
LOG(INFO) << "MC-TRUTH " << !configcontext.options().get<bool>("disable-mc");
bool mctruth = !configcontext.options().get<bool>("disable-mc");
and is passed to every device, e.g.
specs.emplace_back(o2::itsmft::getITSDigitizerSpec(fanoutsize++, mctruth));
// connect ITS digit writer
specs.emplace_back(o2::itsmft::getITSDigitWriterSpec(mctruth));

So, you just need to add to getZDCDigitizerSpec and getZDCDigitWriterSpec an argument (bool usMC) and if it is on, skip everything related to MC labels, like others do.

@shahor02
Copy link
Collaborator

shahor02 commented Mar 4, 2021

Only remaining difference is that Digitizer writes a single entry for all events, while raw to digits writes one entry per TF
This is by design.

@cortesep
Copy link
Contributor Author

cortesep commented Mar 4, 2021

Didn't realize that I could change the constructor. Now it should be ok.

Copy link
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.

@cortesep please fix this

/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5585/0/DataFormats/Detectors/ZDC/src/BCData.cxx:19:17: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5585/0/DataFormats/Detectors/ZDC/src/BCData.cxx:21:7: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5585/0/DataFormats/Detectors/ZDC/src/BCData.cxx:39:12: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5585/0/DataFormats/Detectors/ZDC/src/BCData.cxx:67:12: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5585/0/DataFormats/Detectors/ZDC/src/BCData.cxx:75:12: error: statement should be inside braces [readability-braces-around-statements]

and see 1 comment below.

Also, the Steer/DigitizerWorkflow/src/ZDCDigitWriterSpec.h practically duplicates the Detectors/ZDC/workflow/src/ZDCDigitWriterDPLSpec.cxx.
Could you add to the latter the optional MC output, eliminate the one in the Steer/DigitizerWorkflow? Just call it here:

specs.emplace_back(o2::zdc::getZDCDigitWriterDPLSpec());
as:

specs.emplace_back(o2::zdc::getZDCDigitWriterDPLSpec(false));

Copy link
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.

@cortesep OK, I think we can squash/merge it already, if fixes will be needed, will do them in separate PR.

@shahor02
Copy link
Collaborator

shahor02 commented Mar 5, 2021

No, there are conflicts, could you rebase to fresh dev and resolve the conflicts?

@davidrohr
Copy link
Collaborator

Hi @shahor02 there are only rebase conflicts, you can squash merge already now.
Rebasing doesn't work since apparently there were merge commits in between, that solved some conflicts in between, so just rebasing the individual commits is not possible.

@shahor02 shahor02 merged commit 9fcc374 into AliceO2Group:dev Mar 5, 2021
@shahor02
Copy link
Collaborator

shahor02 commented Mar 5, 2021

@davidrohr right, thanks!

@cortesep cortesep deleted the working_branch branch June 24, 2021 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants