-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Access edm::event from GeneratorInterface #28636
Comments
A new Issue was created by @wouf . @davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
assign generator |
assign generators |
New categories assigned: generators @alberto-sanchez,@SiewYan,@qliphy,@efeyazgan,@mkirsano,@agrohsje you have been requested to review this Pull request/Issue and eventually sign? Thanks |
Any ideas on this? |
@wouf Would you please give more details about this issue? What is embedding mode and why this was not used before? and do you have a recipe to reproduce the error? |
@qliphy The embedding mode on generator level, is needed to mix heavy ion events: to do it we have to care about impact parameter and event plane angle - it have to be the same for both events. To mix two MinBias samples we have to generate the second one using the first as underlying. For HYDJET, on each event (before generating) we have to read underlying sample event like To reproduce it with HYDJET one have to use something like Or one can just look through BaseHadroniser code and realize that we have just empty edm::event, before we care about setEDMEvent. |
Thanks! Can you point us to the input file? |
@qliphy It may be any generated in HepMC format with hi section. I created it with: |
Thanks @wouf I tried to reproduce your error: After producing an input file, and then proceed to mix the events, I got Then I added in the python file as below: (2) then I got Have you already added consumes syntax to replace getByLabel by getByToken? |
I added one to #28764 |
Thanks @wouf cmsrel CMSSW_11_1_0_pre1 /tmp/qili/XXX/CMSSW_11_1_0_pre1/src/GeneratorInterface/HydjetInterface/src/HydjetHadronizer.cc: In constructor 'gen::HydjetHadronizer::HydjetHadronizer(const edm::ParameterSet&)': |
@qliphy I'm sorry, I committed wrong project. Now fixed. |
Thanks, now I get other error messages as below: /cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/gcc/8.2.0-pafccj/bin/../lib/gcc/x86_64-unknown-linux-gnu/8.3.1/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc820/src/GeneratorInterface/HydjetInterface/src/GeneratorInterfaceHydjetInterface/HydjetHadronizer.cc.o: in function |
The version below 1.9.1 does not support "embedding mode" |
@qliphy were you able to reproduce the issue? |
Not yet, Still trying. |
@qliphy I can provide my sequence for local testing if You need |
@wouf that would be useful indeed. |
@qliphy sorry for the late replay On lxplus7 (in the test folder) please do: Then run the script: Then get hydjet 1.9.1 interface: Then setup hydjet libs: To check if it is really version 1.9.1, please run the test: |
@wouf I confirm I can reproduce the error. (1) In Hydjet_Quenched_MinBias_5020GeV_cfi_GEN_SIM_PU.py, there is (2) The relevant pgen path is defined as here: (3) and thus the mixing module runs after generator path (4) All these should explain why currently getEDMEvent gets empty, and probably setEDMEvent should be called after/in the mixing module, e.g.: Hope software experts @davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 @mkirsano @alberto-sanchez @SiewYan can have a look and give more suggestion or help. |
@makortel FYI |
I'm pretty sure I'm missing something so please correct me
From the discussion above it is not clear to me what exactly is the problem. Is it that |
Thank You for the explanation! Тhat doesn't sound comforting... |
I agree. I don't have good ideas for which way to proceed. Is there any hope for getting Hydjet thread-friendly (like Pythia8 alone is, assuming Hydjet is not yet)? How much time does the GEN step take compared to SIM step? |
The current Hydjet family 1.x and 2.x uses Pyquen 1.x as a part. Pyquen family 1.x is based on Pythia 6 (PYQUEN mean PYthia QUENched). As far as I know no way to make Pythia 6 thread-friendly (at least due to fortran 77 specification with SAVE variables, right?).
On 10 events GEN workflow spend 0m53s, GEN-SIM - 60m50s.
I guess it's related to HEPEVT COMMON-block size, which now is 4000, what may be not enough for HI. For now I'm trying to manage it, but with no success yet: changing |
Dear @makortel and @Dr15Jones , for
Could You please to help with it? |
@wouf Is your question about the second occurrence of
? |
With 60-to-1 ratio naively we could be able to feed 60-wise concurrency in SIM with 1 instance of GEN (ignoring edge effects etc). At least for short-to-mid term I would imagine that to be acceptable (assuming there is no filtering of events between GEN and SIM). |
No, about the first one. As far as I understand this strategy, first mix is for background, second - for signal without background (to avoid file reading). Then we just overlay it. |
Yes, more important is to understand why we have warnings from |
@wouf Thanks for the clarification.
I took a look of the One option would be to remove these two if they are not needed (I can't tell). Another option would be to continue to mix the digis in the DIGI step, i.e. not along Another option would be to mix the digis along Perhaps the best strategy would be to get the full chain working first before trying to optimize? (i.e. second option above) |
If I understand correctly, this point suppose to use two synchronized GEN-SIM collections (background and signal) as input for DIGI step? In this case the events should be mixed head to head, samples have to have the same number of events, what would be useless, I guess. |
(assuming we are discussing about the third option above, I wasn't fully sure) Two collections of SimTracks yes, but in the same Event. |
@makortel Sorry for this misunderstanding, I mean second point above:
I'm not sure about it.
If I understand correctly, this point suppose to use two synchronized GEN-SIM collections (background and signal) as input for DIGI step? In this case the events should be mixed head to head, samples have to have the same number of events, what would be useless, I guess.
Do You mean something like this?
|
Can't they use |
Isn't that what the "embedding mode" workflow is doing now, i.e. run MixingModule first in GEN (to overlay gen products), and then again (in playback mode) in DIGI to overlay the SIM products. Or at least that was my understanding based on
Along that way, but I would not be surprised if MixingModule would need something additional to work properly to skip the signal products (I didn't look), and certainly more work would be needed to properly create TrackingParticles from the overlaid signal+background SimTrack collections. |
Possibly in theory, if the background event is the one who drives the vertex position (I can't remember if that was the case). In practice some (possibly heavy) code development would be needed to achieve that. |
Well, if we trying to use standard workflow, pgen for GEN-SIM should looks like:
here the first
So, the game is not worth the candle? |
I assumed this is what You meant in the #28636 (comment) Isn't it? Is it long-term plan? |
The second |
Yes, I meant that. But at that time I was not aware of the circular dependence of
I am not really in a position to tell. I suppose it is up to the simulation L2's and likely eventually on the manpower to do it. |
I guess it needs signal in CF as well? Or maybe I'm doing something wrong, but with
I get:
P.S. - worflows for GEN-SIM:
for DIGIRAW:
|
I see, thank You for explanation.
Do You know, who may answer more exact? |
Sorry, my mistake! With
|
No, MixingModule reads all of its input from "regular" containers (for both signal and pileup events). CrossingFrame is an output product of the MixingModule (mainly used by muon digitizers). Do you have all the era customizations from cmssw/SimGeneral/MixingModule/python/mixObjects_cfi.py Lines 226 to 251 in 0dd520d
?
Technically it would be simulation L2s @civanch and @sbein. Depending on the expected improvements in the workflow execution in grid I might be able to help, but given the extent of development that would require planning. |
Could You please to clarify, what is the "regular" containers? Thanks!
I guess none of them. We are using
I see, thanks. |
The containers produced by
The missing product error you showed in #28636 (comment) was about hits in GEM, which in |
Thank You! |
Thank You @makortel , @qliphy , @Dr15Jones , @gkrintir , @mandrenguyen , @stepobr , all! |
Three HI generators has an issue with edm::event access in embedding mode. It trying to read empty edm::event using BaseHadroniser::getEDMEvent function:
Pyquen:
https://github.com/cms-sw/cmssw/blob/master/GeneratorInterface/PyquenInterface/src/PyquenHadronizer.cc#L125
Hydjet2:
https://github.com/cms-sw/cmssw/blob/master/GeneratorInterface/Hydjet2Interface/src/Hydjet2Hadronizer.cc#L491
Hydjet:
https://github.com/cms-sw/cmssw/blob/master/GeneratorInterface/HydjetInterface/src/HydjetHadronizer.cc#L195
To correct it it is needed call BaseHadroniser::setEDMEvent somewhere, but for now I can't to figure out how to access edm::event (from HiMixGEN) in GeneratorFilter before event generation.
The text was updated successfully, but these errors were encountered: