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

Fix the delay of the CPPF DAQ data #38974

Merged
merged 4 commits into from Aug 11, 2022

Conversation

mileva
Copy link
Contributor

@mileva mileva commented Aug 5, 2022

PR description:

To fix the CPPF DAQ delay and RPC offline clusters BX association.

PR validation:

Tested with:
runTheMatrix and three different workflows:
136.897_RunCosmics2021CRUZET+RunCosmics2021CRUZET+RECOCOSDRUN3+ALCACOSDRUN3+HARVESTDCR3
140.103_RunSingleMuon2022B+RunSingleMuon2022B+HLTRUN3+RECONANORUN3+SKIMSINGLEMUONRUN3
140.117_RunCosmics2022B+RunCosmics2022B+HLTRUN3+RECOCOSMRUN3+SKIMCOSMICSRUN3

And also with private validation and offline analysis.

More details and validation plots might be found here.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

None, to be backported in 12_4_X

Mention @dilsonjd @jhgoh @andresib

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38974/31428

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2022

A new Pull Request was created by @mileva for master.

It involves the following packages:

  • EventFilter/RPCRawToDigi (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@@ -217,7 +220,7 @@ void RPCCPPFUnpacker::processRXRecord(RPCAMCLink link,
return;
}

if (bx < bx_min || bx > bx_max) {
if ((bx - cppfDaq_Delay) < bx_min || (bx - cppfDaq_Delay) > bx_max) {
Copy link
Contributor

@jpata jpata Aug 5, 2022

Choose a reason for hiding this comment

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

wouldn't it make sense to define e.g.

auto bx_corrected = bx - cppfDaq_Delay;

and use it everywhere where the shifted value must be used? does the meaning of bx==0 change or stay the same (e.g. a few lines below here)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jpata

does the meaning of bx==0 change or stay the same
Good question! It looks that is needed for the DPG analysis, but could be deeper. Let me have some time to debug and investigate.

Copy link

Choose a reason for hiding this comment

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

auto bx_corrected implemented as suggested.

Copy link
Contributor Author

@mileva mileva Aug 9, 2022

Choose a reason for hiding this comment

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

Hi @jpata !
I have updated #38979, as well.

On the question "does the meaning of bx==0 change or stay the same".
I checked and it does change nothing both in the CPPF DAQ data (attached cppf_rpc_daq_bx_check.png) or in the input to EMTF (l1_L1Stage2EMTF_rpcHitBX.png).
So, we would prefer to not touch it for the moment.
cppf_rpc_daq_bx_check
l1_L1Stage2EMTF_rpcHitBX

In order to test I used 136.897_RunCosmics2021CRUZET workflow, but I changed the input data. The default run for this workflow is 344518, however in this run the CPPF key were not enabled to EMTF. (I.e. - no RPC input to EMTF, just DAQ data.) Because of this I changed with another cosmics run 356088.
runTheMatrix test ran without problems.

However, I decided to run with more events - the default are 100 events.
And on the 102th event the Module: L1TStage2CPPF:l1tStage2Cppf crashed with segmentation violation.
But I don't think the crash is caused of our update,
as the same module already was reported to crash with release validation after it was already merged, further to latest comments in #38564 .
ping @ram1123 and @zhangcg123

I can give more details during the meeting.

@jpata
Copy link
Contributor

jpata commented Aug 5, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3adec0/26653/summary.html
COMMIT: b900afa
CMSSW: CMSSW_12_5_X_2022-08-05-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38974/26653/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 29 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3691612
  • DQMHistoTests: Total failures: 318
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3691271
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38974/31464

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2022

Pull request #38974 was updated. @jpata, @cmsbuild, @clacaputo can you please check and sign again.

@jpata
Copy link
Contributor

jpata commented Aug 9, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3adec0/26720/summary.html
COMMIT: 70f4195
CMSSW: CMSSW_12_5_X_2022-08-09-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38974/26720/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 27 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3692476
  • DQMHistoTests: Total failures: 313
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3692141
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@mileva
Copy link
Contributor Author

mileva commented Aug 9, 2022

Hi @dilsonjd , @jpata , @zhangcg123
I checked once again with the latest IB, applying the same changes as proposed in this PR - now there is no segmentation violation in the Module: L1TStage2CPPF:l1tStage2Cppf.
Roumyana

@jpata
Copy link
Contributor

jpata commented Aug 10, 2022

I checked once again with the latest IB, applying the same changes as proposed in this PR - now there is no segmentation violation in the Module: L1TStage2CPPF:l1tStage2Cppf.

Sorry I missed the point here - what is the segfault you are talking about?

We can see some differences in RPC distributions

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-08-09-1100+3adec0/52044/validateJR/all_OldVSNew_RunMinimumBias2021wf139p001/all_OldVSNew_RunMinimumBias2021wf139p001c_RPCDetIdRPCRecHitsOwnedRangeMap_rpcRecHits__reRECO_objAT_size.png
image

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-08-09-1100+3adec0/52044/validateJR/all_OldVSNew_RunMinimumBias2021wf139p001/all_OldVSNew_RunMinimumBias2021wf139p001c_RPCDetIdRPCRecHitsOwnedRangeMap_rpcRecHits__reRECO_obj_collection__data__localPosition_x.png
image

which I understand are expected. Surprisingly nothing in muons downstream (too rare?).

I think enabling this change with the Run3_RPC modifier seems reasonable, if it's not planned for Run2 or Phase2.

Are there any studies left you want to do here or is this PR ready?

@mileva
Copy link
Contributor Author

mileva commented Aug 10, 2022

Hi @jpata
There was a segmentation fault caused by the fact I was testing with an old integration build (before the merge of the L1 DQM CPPF module #38564 ), pulling the branch from Dilson, which was created later on. For that period there were another changes in the RPCCPPF unpacker, for which we were not aware. After moving the tests to the latest IB - there were no longer segmentation violation. Sorry, if I introduced some "noise".

About the MinBias - validation plots: Yes, the changes are expected. As a consequence of the "delayed" daq data some of the offline rpc clusters in the endcap were not reconstructed and thus not stored in the data.
And in the same time some "yearly coming" digis (with respect to the usual readout) were propagated. So, I think the comparison plots are correct.

About the muons: in general there are not too much muons in the MinBias data. From the other hand the RPC rechits are not widely used in the reconstruction, but rather for the the timing studies.

Modification is for Run3. We do not need this for Run2 and we don't have plan to explore it in phase 2.

I don't think we can make some more studies at this moment and I would like to ask to approve this pull request.

Thank you!
Roumyana

@jpata
Copy link
Contributor

jpata commented Aug 11, 2022

+reconstruction

  • bugfix for RPC datataking
  • affects RPC quantities in data for Run3 (may have effects on muons downstream)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@jpata
Copy link
Contributor

jpata commented Aug 11, 2022

@mileva can you please edit the title to be the same as you used for the 12_4_X version of this PR? That one is a lot more descriptive.

@mileva mileva changed the title Mileva/cppf delay fix v0 Fix the delay of the CPPF DAQ data for backport to 12_4_X Aug 11, 2022
@jpata
Copy link
Contributor

jpata commented Aug 11, 2022

just to clarify, you can remove 12_4_X from the title :)

@qliphy qliphy changed the title Fix the delay of the CPPF DAQ data for backport to 12_4_X Fix the delay of the CPPF DAQ data Aug 11, 2022
@qliphy
Copy link
Contributor

qliphy commented Aug 11, 2022

+1

@cmsbuild cmsbuild merged commit 4600eda into cms-sw:master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants