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

TPC: Add possibility to simulate distortions #1507

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

wiechula
Copy link
Collaborator

Requires adding --tpc-distortion-type 2 in the anchorMC script to remainingargs not done in this PR.

Copy link

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2022-pp-apass4
async-2023-pbpb-apass
async-2023-pp-apass1
async-data
async-mc
async-2022-pp-apass6

@wiechula wiechula force-pushed the master branch 2 times, most recently from 8daa79a to b145fc3 Compare March 1, 2024 15:39
@wiechula
Copy link
Collaborator Author

wiechula commented Mar 4, 2024

@chiarazampolli , @sawenzel , shall we go ahead with this?

@chiarazampolli
Copy link
Collaborator

Hello @wiechula ,
I would let @sawenzel and @benedikt-voelkel review in detail. For now, I have a question: you checked thoroughly that the use of rate.second is what you need, then scaled by 2.414, right?

@wiechula
Copy link
Collaborator Author

wiechula commented Mar 4, 2024

@chiarazampolli , I didn't check the numbers in detail. But rate.second are the scalers (ZDC for PbPb) if I understand correctly. And to scale to pp in PbPb we should multiply by 2.414. Unless I overlook something.

MC/bin/o2dpg_sim_workflow.py Outdated Show resolved Hide resolved
MC/bin/o2dpg_sim_workflow.py Show resolved Hide resolved
MC/bin/o2dpg_sim_workflow_anchored.py Outdated Show resolved Hide resolved
MC/bin/o2dpg_sim_workflow_anchored.py Outdated Show resolved Hide resolved
MC/bin/o2dpg_sim_workflow_anchored.py Outdated Show resolved Hide resolved
# in case of PbPb the conversion factor ZDC ->FT0 (pp) must be set
tpc_corr_options_mc=''

if tpcDistortionType == 0: # disable distortion corrections
Copy link
Contributor

Choose a reason for hiding this comment

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

In anchored simulations I think these configurations might clash with some we would derive from the anchoring and pass from the outside. I think to be discussed with @sawenzel as well.

rate = ctpscaler.getRateGivenT(finaltime, ctpclass, ctptype)
#if ColSystem == "PbPb":
# rate.first = rate.first / 28.
# rate.second = rate.second / 28.

print("Global rate " + str(rate.first) + " local rate " + str(rate.second))
scaler = None
if rate.second >= 0:
scaler = rate.second
if rate.first >= 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

@chiarazampolli do you know why we would only return the final/corrected local rate if the global rate is rate.first >= 0? I don't see it being used inside this branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ciao @benedikt-voelkel ,

I don't know, I am not the author of this part of the code. But I guess that if rate.first is 0, then also rate.second is 0. So it is ok to return none none. Still I agree that it is not very clear.

Chiara

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was it maybe added here: #1333?

@sawenzel
Copy link
Contributor

sawenzel commented Mar 7, 2024

Sorry for the delay. I'll take a look asap.

Comment on lines +1018 to +1020
tpcLocalCFreco['TPCCorrMap.lumiInstFactor'] = str(lumiInstFactor)
tpc_corr_options_mc=' --corrmap-lumi-mode 2 '
tpcLocalCFreco['TPCCorrMap.lumiInst'] = str(CTPSCALER)
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not correspond to what was done for digitization.
Here, TPCCorrMap.lumiInst and TPCCorrMap.lumiInstFactor are set separately whereas for digitization TPCCorrMap.lumiInst is set directly to the factor.

Is it because the configKey values are digested differently in reconstruction and digitization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, the digizer does not use the lumiInstFactor https://github.com/AliceO2Group/AliceO2/blob/0129938292bf379849ec58652be5c1cfa88e60f1/Detectors/TPC/simulation/src/Digitizer.cxx#L227
While to correction map in reco does: https://github.com/AliceO2Group/AliceO2/blob/0129938292bf379849ec58652be5c1cfa88e60f1/Detectors/TPC/calibration/src/CorrectionMapsLoader.cxx#L201
For reconstruction I wanted to mirror what is done in data. But I assume one could also directly multiply here, right @shahor02 ?


lumiInstFactor=1
if COLTYPE == 'PbPb':
lumiInstFactor=2.414
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no other way to obtain this magic number from some central instance?

@sawenzel
Copy link
Contributor

I have tested this PR and it works for run 544013 (in the range 544013 - 544126).

There was slight glitch with defaults-o2-epn (segfault in ROOT access to CCDB objects) on the EPNs but with default-o2 it is ok. The PR also doesn't modify default behaviour since distortions in digitization need to be enabled with --tpc-distortion-type in the arguments to the anchoring script.

From my point of view this can be merged.

@benedikt-voelkel benedikt-voelkel added async-2023-pbpb-apass3 async-2023-pbpb-apass4 Request porting to async-2023-pbpb-apass4 labels Apr 24, 2024
@benedikt-voelkel benedikt-voelkel merged commit 7188d08 into AliceO2Group:master Apr 24, 2024
6 checks passed
@chiarazampolli
Copy link
Collaborator

Hello @wiechula ,

Following what we said just now at the WP12+13 meeting, just to be sure: @sawenzel did the test with run 544013, did this run have the CCDB for the test? Or do we have it only for runs in:

544116
544121
544122
544123
544124
?
Chiara

@wiechula
Copy link
Collaborator Author

@chiarazampolli , the runs covered are 544013 - 544126, see O2-4531.

@chiarazampolli
Copy link
Collaborator

Good, thanks!

benedikt-voelkel pushed a commit that referenced this pull request Apr 26, 2024
benedikt-voelkel pushed a commit that referenced this pull request Apr 26, 2024
@benedikt-voelkel benedikt-voelkel removed the async-2023-pbpb-apass4 Request porting to async-2023-pbpb-apass4 label May 7, 2024
benedikt-voelkel pushed a commit that referenced this pull request May 16, 2024
Co-authored-by: Sandro Wenzel <[email protected]>
(cherry picked from commit 7188d08)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants