-
Notifications
You must be signed in to change notification settings - Fork 499
dpl-workflow: Add missing flags to disable ctp-lumi-request #15446
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
Merged
+2
−2
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidrohr @fweig Does o2-tpc-reco-workflow ever need the
tpc-scalerworkflow if theoutput-typeis nottracks. And the same for the CTPLumi (used only to scale TPC tracks syst errors).In opposite, for the same reason, if in the
o2-gpu-recothe we add--disable-ctp-lumi-request(e.g. because the scaling is done with IDC), how the TPC syst errors scaling will be done? https://github.com/fweig/AliceO2/blob/cba61dce62a1a213e20e68581ffc5c0adf225825/prodtests/full-system-test/dpl-workflow.sh#L615There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahor02 : if i inderstand correctly, this fix was suggested by @matthias-kleiner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @davidrohr @shahor02 ,
If the o2-tpc-reco-workflow does not have output type tracks, then I think in principle you also dont need the tpc-scaler-workflow. If I understand correctly the tpc-scaler-workflow is now always added when we run with TPC. So if you run only with output type cluster and dont need the tracks, then we could disable the tpc-scaler-workflow. But then you would still need to set the
--disable-ctp-lumi-requestin the tpc-reco-workflow.I think we set the
--disable-ctp-lumi-requestonly in case CTP is not available or? It should not be used for switching to IDC scaling in the tpc-scaler-workflow. The scaling source for the corrections in the tpc-scaler-workflow should always be set with theaddOption(options, ConfigParamSpec{"lumi-type", o2::framework::VariantType::Int, 0, {"1 = use CTP lumi for TPC correction scaling, 2 = use TPC scalers for TPC correction scaling"}});https://github.com/AliceO2Group/AliceO2/blob/babe031809ef7185a73c2a04ecaa66eff1b95df2/Detectors/TPC/calibration/src/CorrectionMapsOptions.cxx#L47`Before we made the PR with all the changes for the map merging we had only that one option
--disable-ctp-lumi-requestthat disabled the CTP for correction maps scaling and the cluster errors and since both were done in one workflow we needed this option only once. But now we need it for the scaler-workflow and the tpc-reco-workflow. But perhaps there is also a better way to do that...So to my understanding this PR should be correct, but we could disable the tpc-scaler-workflow if we dont need TPC tracks. Or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @matthias-kleiner! My concern is not this PR per se (except that we could eliminate the tpc-scaler process from the o2-tpc-reco in absence of tracks output and always suppress the ctp input) but the way input of the ctp lumi to o2-gpu-workflow (and other workflows dealing with TPC syst.errors) is organized: it is true that currently the setenv-extra adds
--disable-ctp-lumi-requesttoTPC_CORR_SCALINGonly in absence of the CTP (thus, adding a redundant input in case of the IDC scaling), but I don't like that if for some reason we restrict the tpc-scalers input to strictly needed ones, then the TPC syst.errors scaling will be broken. I would disable the request for CTP inputs in these workflows only in the absence of CTP detector directly.BTW, now I realized that the TPC errors scaling is incorrect already now: the
$TPC_CORR_KEYis added totpc-scaler, and matching workflows, but not the too2-gpu-reco. Hence, in the case of PbPb, theTPCCorrMap.lumiInstFactor=2.414is not passed to the o2-gpu-reco and the scaling is done at the face value of the ZDC lumi counter. I'll add it, but I think the most robust way (working also in the absence of the CTP) would be to force the tpc-scaler to send a dedicated "pp-equivalent" lumi scaler, which the downstream processes would subscribe to instead of the CTP lumi. In the presence of the CTP, it could send the scaler equivalent to what we have today (ctp_lumi * TPCCorrMap.lumiInstFactor). When CTP is absent, it could send the IDC scaler rescaled by the appropriate factor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the current handling of the CTP scaling is somewhat fragile. Sending a dedicated pp-equivalent luminosity scaler that other workflows consume would indeed be cleaner.
Looking at the code, it seems that instLumi is already stored as the scaled value in tpc-scaler and forwarded downstream. Am I missing where o2-gpu-reco bypasses this and uses the raw CTP lumi instead?
The instLumi is stored as the scaled value here:
AliceO2/Detectors/TPC/calibration/src/CorrectionMapsLoader.cxx
Line 82 in babe031
and then forwarded here:
AliceO2/Detectors/TPC/workflow/src/TPCScalerSpec.cxx
Line 175 in babe031
In that case, renaming it as you suggested would also make sense. Then the only missing part would be the conversion from IDC to the corresponding pp-equivalent luminosity value, if I understand correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Indeed, I did not check that now global matchers and gpu reco. get the lumi output of the tpc-scaler, rather than accessing the CTP input directly and rescaling it if needed. But then, we don't need to pass the $TPC_CORR_KEY to the global matchers (which confused me).
Indeed, as it is now, we just need to emulate with the IDC data a scaled CTP lumi in case the CTP is absent.
What I would do additionally is to avoid adding the tpc-scaler-process from the TPC workflows and global matchers in the
--disable-root-inputsmode: in the dpl-workflow driven workflows, it is already added as a separate workflow, in the MC it will be added together with readers when needed, as all dependencies.