cms-l1t-offline / cmssw

CMS Offline Software
cms-sw.github.io/cmssw
Apache License 2.0
17 stars 27 forks source link

Missing BMTF muons from Ntuple when running on MC Run3 #886

Open rekovic opened 3 years ago

rekovic commented 3 years ago

Recipe to reproduce the problem is already listed in the issue https://github.com/cms-l1t-offline/cmssw/issues/871

panoskatsoulis commented 3 years ago

BMTF modules in the cms.Schedule:

The empty bmtf tree in the Ntuples is due to the 2nd point in the list above. A problem is located in the BMTF Emu inputs coming from the TwinMux and bmtf receives empty phi-stubs

The 3rd point about the --era modification is treated by this PR #887

BenjaminRS commented 3 years ago

HI @panoskatsoulis thanks for this. So from what I understand from your email the issue lies in the TwinMux unpacker. Where the possible solution is to use a copy of the BMTFInputs unpacker and unpack the data we want? What is the next step? Does @dinyar agree with this course of action? Who do we need to sign off on it?

dinyar commented 3 years ago

Hi! I think it depends a bit whether we're expecting changes to anything before the BMTF in the reemulation. If I understand correctly if we use bmtfDigis as input to the BMTF we don't fully re-emulate the chain anymore because the bmtfDigis are generated once during the initial MC campaign and then kept as is. I think medium term it's definitely better if we use the unpacked DT outputs as the input to the TwinMUX (instead of the unpacked TwinMUX inputs that seem to be empty at the moment) as suggested by George in the email I forwarded.

Cheers, Dinyar

panoskatsoulis commented 3 years ago

Hi @BenjaminRS, the suggestion to use the BMTF unpacker to unpack TM feds is just a consideration and needs some thought before being implemented.

I think a quick solution, especially since this issue is considered urgent as @bundocka explains in the mail thread, is to use DTs output and bypass TM for now (as George and Dinyar both suggest).

If all agree with this solution I can prepare a fix and open a PR, inform me

BenjaminRS commented 3 years ago

Hi @dinyar and @panoskatsoulis thanks for the inputs - to me this suggestion of using the unpacked DT outputs as the input to the TwinMux would seem like a good fix for now. I don't know who else needs to sign off on it for it to be done. @panoskatsoulis can you go ahead and make a PR in the mean time please?

panoskatsoulis commented 3 years ago

So I tried to use the DT inputs for re-emulating BMTF but I got empty trees again. Seems to me that the problem goes deeper (in the L1 re-emu) and neither the DT collection can be used.

However, the packed inputDigis from the BMTF packer (during DIGI2RAW) is actually the TM output simulated already [1]. So it seems safe to me to use the output of the BMTF unpacker since, in reality, we will be using simulated TM output. Except I'm missing smth.

I've made the minimal changes needed for fixing the Run3 ONLY reemulation [2], if people agree that this can be a solution I'll open a PR and also we can extend this to Run2 later.

[1] https://github.com/cms-l1t-offline/cmssw/blob/l1t-integration-CMSSW_11_2_0/EventFilter/L1TRawToDigi/python/bmtfStage2Raw_cfi.py#L9 [2] https://github.com/cms-l1t-offline/cmssw/compare/l1t-integration-CMSSW_11_2_0...panoskatsoulis:fix_empty_bmtf_ntuples?expand=1

panoskatsoulis commented 3 years ago

one more detail - I just saw that OMTF actually does the same for the DT input in the re-emulation takes the input from their packed products that, in reality, is the simulated TwinMux from the MC production

dinyar commented 3 years ago

Hi Pano,

Just to make sure, did you do something like

stage2L1Trigger.toModify(process.simTwinMuxDigis,
        DTDigi_Source      = 'simDtTriggerPrimitiveDigis',
        DTThetaDigi_Source = 'simDtTriggerPrimitiveDigis'

? It looks like this is already done in some circumstances[1]. (I don't know what run3_GEM is.. )

As an alternative I had a bit of a look into how this was done before and I you may have meant this, but just in case: It looks like it would be an option to use the bmtfDigis as the input to the TwinMUX, see e.g. [2]. I'm far from an expert, so it's not entirely clear to me if this would be reasonable, but we would have the TwinMUX re-emulation back which we would lose if we use the bmtfDigis directly as the input to the BMTF.

Please correct me if I'm missing something, I haven't had much time to think about it today.

Cheers, Dinyar

[1] https://github.com/cms-l1t-offline/cmssw/blame/l1t-integration-CMSSW_11_2_0/L1Trigger/Configuration/python/customiseReEmul.py#L210-L214 [2] https://github.com/cms-l1t-offline/cmssw/blob/cccf6b47bf3e0ba264ced107a141d03854de51a2/L1Trigger/Configuration/python/customiseReEmul.py#L131-L132

panoskatsoulis commented 3 years ago

Hi Dinyar, I made exactly the changes you write in your first snipper above. It seems to me that the problem is the input of the TM and not in some "TM unpacker".

Below are my thoughts, if I'm missing something please share your view.

About the alternative to feed the Twinmux with the bmtfDigis is a bit confusing I think. From my investigation, I saw that bmtfDigis is actually the Twinmux output packed by the BMTF Packer during step DIGI2RAW [1]. Now, if the TwinMux is shifting stubs in time what is gonna be the output of the re-emulated Twinmux this way? Smth like stubs shifted twice? (I think we need someone who understands TM to understand completely if this could be a sensible option)

The real question is... do we really want to emulate TM or just re-emulating the TFs is good enough? At the end of the day, we are already skipping re-emu TM for the OMTF branch [2] on multiple occasions (in Run 3 too since omtf is not modified for Run3_GEM).

[1] It's easy to locate it actually, I run cmsDriver.py ... -s DIGI2RAW --era Run3 ... and then I ran edmConfigDump <cfg>, finally the line for what I see in the link below https://github.com/cms-l1t-offline/cmssw/blob/l1t-integration-CMSSW_11_2_0/EventFilter/L1TRawToDigi/python/bmtfStage2Raw_cfi.py#L9

[2] https://github.com/cms-l1t-offline/cmssw/blob/l1t-integration-CMSSW_11_2_0/L1Trigger/Configuration/python/customiseReEmul.py#L159-L163 (just one example case)

dinyar commented 3 years ago

Hi Pano,

Ok, that's too bad. My last suggestion along those lines would be to try the muonDTDigis collection. That's what the DT unpacker produces, so we'd get something from quite early in the chain to re-emulate the TwinMUX from there.

I think you make a fair point regarding the re-emulation of the TwinMUX, at the moment I'm not aware of any changes to their emulator, so it's fairly safe to skip it in re-emulation. Medium term it would make me nervous to have that bypass though because these are things that tend to get forgotten until someone ends up spending days debugging why their changes to that system aren't having effects.. (Regarding the OMTF: Here the bypass is intended because to my knowledge the TwinMUX merely forwards the DT data without modifying it.)

I think you're also right that we shouldn't experiment with the bmtfDigis if we don't know what the TwinMUX would do if it runs a second time on them.

Cheers, Dinyar

panoskatsoulis commented 3 years ago

Hi all, Dinyar thx for the suggestion. It's not working either, something is spooky with the DTs and its products I think. I get errors for missing input products.

For not freezing the menu studies, I opened a PR using the bmtfDigis as input to the BMTF emulator BUT as you said we have to revisit this soon. I agree that if we leave it as is (bypassing TM re-emu) someone will have a problem with this in the future.

Here is the PR available, https://github.com/cms-l1t-offline/cmssw/pull/888 if all agree it can be used temporarily (even by merging it in the recipe and not by merging the PR). Panos

bundocka commented 3 years ago

Hi all,

Thanks a lot Panos and Dinyar for following up quickly on this.

Looks like a tricky problem but I think for now for the menu studies, (and for the scouting studies) bypassing the TM emulator if there have been no recent changes will be ok.

Comparing the unpacked vs emulated BMTF muons after merging this PR I see they are identical for 5000 events.

I think it's a good idea just to get people to manually merge this branch in the Run 3 workflow like you said rather than merge this change to the integration branch, and avoid potential problems in the future.

Unless there are any objections I'll update the instructions and notify the menu group and reply to the scouting thread with the updated workflow.

Thanks again for the prompt response!

Cheers, Aaron

On 22 Feb 2021, at 13:27, Panos notifications@github.com wrote:

Hi all, Dinyar thx for the suggestion. It's not working either, something is spooky with the DTs and its products I think. I get errors for missing input products.

For not freezing the menu studies, I opened a PR using the bmtfDigis as input to the BMTF emulator BUT as you said we have to revisit this soon. I agree that if we leave it as is (bypassing TM re-emu) someone will have a problem with this in the future.

Here is the PR available, #888 https://github.com/cms-l1t-offline/cmssw/pull/888 if all agree it can be used temporarily (even by merging it in the recipe and not by merging the PR). Panos

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-l1t-offline/cmssw/issues/886#issuecomment-783372981, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFJCT74YP6SNXEZ3GP22STTAJLUPANCNFSM4XYUHSYA.

panoskatsoulis commented 3 years ago

I agree, and also it's better to leave this issue open and link it to the Offline SW gDoc. Otherwise, it's gonna be forgotten Cheers, Panos

rekovic commented 3 years ago

Ping Carlo Batilliana and Muon DPG.

rekovic commented 3 years ago

Ping @battibass.

battibass commented 3 years ago

Hi @rekovic, I has started actually looking into this right now (in parallel with your ping). Can you confirm that the correct recipe for testing is the one listed here?

battibass commented 3 years ago

Hi again, I had a look at the sequence from the workflow quoted at the beginning of this thread and, if you want to emulate the DT trigger-primitives, and then the TwinMux, you should for sure change the DT local trigger emulator input from simMuonDTDigis to muonDTDigis as in:

process.simDtTriggerPrimitiveDigis.digiTag = cms.InputTag("muonDTDigis")

because you are running on unpacked RAW data.

From my test on limited statistics this makes the BX distribution of the theta DT primitives consistent with the one from bmtfDigis (modulo the fact that bmtfDigis provides only primitives in a subset of the BXes produced by emulator because of the reduction on the BX readout width from packing -> unpacking).

For the TwinMux (phi) primitives I see small differences, that make me think one should validate the use of RPC information in the TwinMux, but better asking an RPC expert to ensure that the unpacking for RPC is correct.

I am happy to iterate further if needed.

battibass commented 3 years ago

Btw, just a reminder: re-emulating the DT local trigged from DT digis is a reasonable thing to do in MC, but I would advise to avoid doing that with real data. As discussed many times in the past, in fact, in real data the input of the trigger is not exactly consistent with what is readout for offline reconstruction. For this reason, a 100% data-emulator agreement is not expected, independently from the accuracy of the emulation logic.

battibass commented 3 years ago

Ping @rekovic , should I do anything else?

panoskatsoulis commented 3 years ago

Hello @battibass, we're looking into this your instructions were really helpful thanks a lot We'll be back in case of need

battibass commented 3 years ago

OK, very good, thanks for the quick reply!

ckamtsikis commented 3 years ago

Hello @battibass ,

battibass commented 3 years ago

Hello @ckamtsikis , firstly, I used the recipe reported here without any further addition. Based on that workflow, I customised the inputs to the emulation and to the ntuples to find what to change. The products I've compared were the theta and phi DT trigger primitives, looking for differences in the ntuple content, if the TPs come from:

and I run on the same events.

The point is that the TwinMux does nothing with the theta TPs, only performs logic on the phi TPs, so, correctly emulating the DT local trigger (before the TwinMux) is enough to get perfect agreement between bmtfDigis and simDtTriggerPrimitiveDigis, which is what I observe.

Instead, when I compare the phi TPs from bmtfDigis and simTwinMuxDigis I see "small" differences I did not debug further in the BX distribution of the two collections, in principle I was hoping for "identical" results (at least in the BX range that is common between bmtfDigis and simTwinMuxDigis).

Does this clarify my check?

panoskatsoulis commented 3 years ago

Hi let me jump in here, @battibass do you know how to configure the TwinMux Emu (simTwinMuxDigis) to run with a dummy null RPC input? Is it possible/easy?

This way we could quickly check if the little discrepancies come from the RPC as you said. Also, thx for explaining further.

battibass commented 3 years ago

@panoskatsoulis : one alternative I could suggest is to add to the L1TNtuple a variable to check the use of RPC information in the TwinMux. I think this information gets lost in the BMTF packing/unpacking step (i.e. it is always 0), but is available when one re-emulates the TwinMux. If you don't want to add permanently this variable to the ntuples, you could do that only for testing purposes right now.

Meanwhile, I experimented a little further, and I think I get a BX distribution for the re-emulated phi TPs that matches the bmtfDigis if I add this line to my setup:

process.simTwinMuxDigis.RPC_Source = cms.InputTag("simMuonRPCDigis")

Though, I am not sure this is the best way to retrieve RPC digis to be used by the TwinMux.

ckamtsikis commented 3 years ago

Hi @battibass ,

I am very sorry for the late response. I reproduced and found that differences exist in the BX distributions [1] between the bmtf unpacker ( blue ) and the simTwinMux ( red ). Changing the rpc input to the simTwinMux from rpcTwinMuxRawToDigi to the simMuonRPCDigis I can see [2] that the two plots are identical for the bmtf BX range (-2,2).

I think we need further help by some RPC expert!

[1]: , [2]:

battibass commented 3 years ago

@ckamtsikis no problem (I also am replying with some delay). Likely you have pinged them over e-mail, but let me ping few RPC experts directly from this issue: @mileva @jhgoh @andresib

mileva commented 3 years ago

@ckamtsikis ,you can use simMuonRPCDigis. The unpacked from the TM are expected to be the same. I don't know the reason for the small differences that you are reporting, as the TwinMux-RPC unpacker it is not changed since it was developed. We will have a look, but for the moment I think the simMuonRPCDigis is a very good solution. Let me ping also @ftorresd and @dilsonjd in case they want to add more comments.

battibass commented 2 years ago

Hello, as I am doing some cleanup of old GitHub issues where I am involved, I profit to ask whether this one still needs to remain open.