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.

Sign up for GitHub

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

Jump to bottom

Adding time info to the rpc points extrapolated from the CSC and DT segments #39115

Merged

cmsbuild merged 7 commits into cms-sw:master from ahussein1194:hussein_branch

Aug 24, 2022

Contributor

ahussein1194 commented Aug 19, 2022

PR description:

The edited plugins are used by the offline RPC DPG analysis and should not affect any other sw packages. In general the RPCs use the RPCPontProducer to extrapolate segments from the near muon detectors and the extrapolated points are converted to objects of type of RPC Rechits, used in the offline efficiency measurements. In this code we updated the timing information to the extrapolated RPC points.

PR validation:

Tested with runTheMatrix script with:

1001.3 RunSingleMuon2022B+TIER0EXPRUN3+ALCAEXPRUN3+ALCAHARVDEXPRUN3
1044.0 RunRawPPS2022B+TIER0EXPPPSCALRUN3+ALCASPLITPPSCALRUN3+ALCAHARVDPPSCALRUN3
140.002 RunZeroBias2022B+HLTRUN3+RECONANORUN3+HARVESTRUN3

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

No backport is needed.

@mileva @jhgoh @andresib


          updated some plugins in RecoLocalMuon/RPCRecHit/

308e446

cmsbuild added this to the CMSSW_12_5_X milestone

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels

Contributor

cmsbuild commented Aug 19, 2022

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39115/31653

This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39115/31653/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39115/31653/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild added code-checks-rejected and removed code-checks-pending labels


          updated some plugins in RecoLocalMuon/RPCRecHit/

fe55343

cmsbuild added code-checks-pending and removed code-checks-rejected labels

Contributor

cmsbuild commented Aug 19, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39115/31654

This PR adds an extra 20KB to repository

cmsbuild added code-checks-approved and removed code-checks-pending labels

Contributor

cmsbuild commented Aug 19, 2022

A new Pull Request was created by @ahussein1194 (Ahmed Hussein) for master.

It involves the following packages:

RecoLocalMuon/RPCRecHit (reconstruction)

@jpata, @cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@bellan, @jhgoh 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

Contributor

jpata commented Aug 22, 2022

@cmsbuild please test

cmsbuild added tests-started and removed tests-pending labels

jpata reviewed

View reviewed changes

Contributor

jpata left a comment •

edited

Loading

To be fixed here:

can you use a more descriptive title for the PR (edit button)?
do these modules run in any workflow (online or offline), or is it "private code" run on an ad-hoc basis
fix magic numbers, commented-out code

For the whole subpackage, in a follow-up (#39131):

cout should be removed from these modules (it looks like this is a common issue for the whole subpackage)
the config parameters should have default values, defined in a fillDescriptions (see e.g. here: 4eb1ef6)

RecoLocalMuon/RPCRecHit/plugins/CSCSegtoRPC.cc Outdated

+                  float myTime = -9999.;
+                  float myTimeErr = -9999.;
+                  int myBx = -99;
                   if (debug)
                     std::cout << "CSC \t loop over all the CSCSegments " << std::endl;

Contributor

jpata Aug 22, 2022

cout is not allowed in production code, it's not thread safe.
please switch to LogDebug.

Contributor

jpata Aug 22, 2022

Contributor Author

ahussein1194 Aug 22, 2022

Now "LogDebug" is being used instead of "cout". Also a fillDescriptions has been defined for the default values. They have been implemented in the last commit.

RecoLocalMuon/RPCRecHit/plugins/CSCSegtoRPC.cc Outdated

                   if (debug)
                     std::cout << "CSC \t loop over all the CSCSegments " << std::endl;
                   for (segment = allCSCSegments->begin(); segment != allCSCSegments->end(); ++segment) {
                     CSCDetId CSCId = segment->cscDetId();
+                    myTime = segment->time();
+                    myBx = round(myTime / 25.);

Contributor

jpata Aug 22, 2022

magic number 25 should be a constexpr. In fact, isn't this number something that should come from the conditions DB?

Contributor Author

ahussein1194 Aug 22, 2022

It is the time difference between bunch crossings. It is now defined as const.

RecoLocalMuon/RPCRecHit/plugins/CSCSegtoRPC.cc

Comment on lines -90 to -91

		//&& acos(dz)180/3.1415926 > 45. && acos(dz)180/3.1415926 < 135.){
		//&& segment->chi2()< ??)Add 3 segmentes in the endcaps???

Contributor

jpata Aug 22, 2022

remove

Contributor Author

ahussein1194 Aug 22, 2022

Already removed.

RecoLocalMuon/RPCRecHit/plugins/CSCSegtoRPC.cc Outdated

Comment on lines 102 to 103

		if (debug)
		std::cout << "CSC \t \t yes" << std::endl;

Contributor

jpata Aug 22, 2022

LogDebug here and below

Contributor

jpata Aug 22, 2022

Contributor Author

ahussein1194 Aug 22, 2022

I replaced "cout" by "LogDebug"

RecoLocalMuon/RPCRecHit/plugins/CSCSegtoRPC.cc

                               RPCGeomServ rpcsrv(rpcId);
                               if (dr > 200. || fabs(dz) > 55. || dfg > 1.) {
-                                //if(rpcRegion==1&&dfg>1.&&dr>100.){

Contributor

jpata Aug 22, 2022

remove

Contributor Author

ahussein1194 Aug 22, 2022

Already removed all lines of code that were commented out.

RecoLocalMuon/RPCRecHit/plugins/DTSegtoRPC.cc Outdated

+                      if (segment->hasPhi()) {
+                        myPhiTime = segment->phiSegment()->t0();
+                        myPhiBx = round(segment->phiSegment()->t0() / 25.);

Contributor

jpata Aug 22, 2022

25 from DB / as constexpr

Contributor Author

ahussein1194 Aug 22, 2022

Now it is defined as const.

RecoLocalMuon/RPCRecHit/plugins/DTSegtoRPC.cc Outdated

Comment on lines 133 to 134

		if (debug)
		std::cout << "segment t0 = " << myPhiTime << "\tround = " << myPhiBx << std::endl;

Contributor

jpata Aug 22, 2022

LogDebug

Contributor

jpata Aug 22, 2022

Contributor Author

ahussein1194 Aug 22, 2022

Now replaced by "LogDebug"

RecoLocalMuon/RPCRecHit/plugins/DTSegtoRPC.cc

Comment on lines +126 to +128

+                      float myPhiTime = -9999.;
+                      float myPhiTimeErr = -9999.;
+                      int myPhiBx = -99;

Contributor

jpata Aug 22, 2022

these placeholder magic number are repeating several times. can you define them in a single place?

RecoLocalMuon/RPCRecHit/plugins/DTSegtoRPC.cc Outdated

Comment on lines 285 to 296

+                        float myPhiTime = -9999.;
+                        float myPhiTimeErr = -9999.;
+                        int myPhiBx = -99;
+                        if (segment->hasPhi()) {
+                          myPhiTime = segment->phiSegment()->t0();
+                          myPhiBx = round(segment->phiSegment()->t0() / 25.);
+                          if (debug)
+                            std::cout << "segment t0 = " << myPhiTime << "\tround = " << myPhiBx << std::endl;
+                        }
+                        if (!(myPhiBx <= maxPhiBX && myPhiBx >= minPhiBX))  //limit only to the RPC readout range
+                          continue;

Contributor

jpata Aug 22, 2022

looks like this is copy-paste from above. can you simplify with a function?

RecoLocalMuon/RPCRecHit/plugins/DTSegtoRPC.h Outdated

@@ @@ -33,6 +34,9 @@ class DTSegtoRPC { @@
                 double MaxD;
                 double MaxDrb4;
                 double MaxDistanceBetweenSegments;
+                //by @ahussein

Contributor

jpata Aug 22, 2022

remove

Contributor Author

ahussein1194 Aug 22, 2022

Done.

ahussein1194 changed the title ~~updated some plugins in RecoLocalMuon/RPCRecHit/~~ updated some plugins in RecoLocalMuon/RPCRecHit/ and added some timing information to the extrapolated RPC points

Contributor

cmsbuild commented Aug 22, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9d6312/26971/summary.html
COMMIT: fe55343
CMSSW: CMSSW_12_5_X_2022-08-21-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39115/26971/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3693040
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3693004
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 added tests-approved and removed tests-started labels

jpata mentioned this pull request

modernization of RPC code #39131

Open

Contributor

mileva commented Aug 22, 2022

do these modules run in any workflow (online or offline), or is it "private code" run on an ad-hoc basis

Hi @jpata
I don't think those are called by some workflow. The modules are called by our "private" offline analysis code. But the aim is to update it step by step and align with the recent cmssw requirements ...

@ahussein1194 will implement the asked changes for this PR. He is working currently on the implementation.
Thanks!
Roumyana

ahussein1194 changed the title ~~updated some plugins in RecoLocalMuon/RPCRecHit/ and added some timing information to the extrapolated RPC points~~ Adding time info to the rpc points extrapolated from the CSC and DT segments


          Adding time info to the RPC points extrabolated from the CSC and DT s…

d2b25b9

…egments

cmsbuild removed the tests-approved label

Contributor

perrotta commented Aug 24, 2022

please test

cmsbuild added tests-started and removed tests-pending labels


          Updated testRPCPointProducer.py

e4be1ca

cmsbuild added code-checks-pending tests-pending and removed tests-started code-checks-approved labels

Contributor

cmsbuild commented Aug 24, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39115/31752

This PR adds an extra 24KB to repository

cmsbuild added code-checks-approved and removed code-checks-pending labels

Contributor

cmsbuild commented Aug 24, 2022

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

Contributor

jpata commented Aug 24, 2022

@cmsbuild please test

cmsbuild added tests-started and removed tests-pending labels

Contributor

cmsbuild commented Aug 24, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9d6312/27043/summary.html
COMMIT: e4be1ca
CMSSW: CMSSW_12_5_X_2022-08-23-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39115/27043/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3693084
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3693054
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

cmsbuild added tests-approved and removed tests-started labels

Contributor

jpata commented Aug 24, 2022

+reconstruction

resign since Adding time info to the rpc points extrapolated from the CSC and DT segments #39115 (comment) after additional issues where caught by Andrea (thanks!)
in any case this is for RPC private usage, these modules are not running in production as far as I can tell, and further cleanup in this package is needed

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels

Contributor

cmsbuild commented Aug 24, 2022

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)

Contributor

perrotta commented Aug 24, 2022

+1

cmsbuild added orp-approved and removed orp-pending labels

cmsbuild merged commit 0a8b6ea into cms-sw:master

Contributor

mileva commented Aug 24, 2022

Thanks a lot for the help @perrotta and @jpata !
Roumyana

cmsbuild mentioned this pull request

[GCC][PATCH] libstdc++: Mark non-exported function always_inline cms-sw/cmsdist#8039

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-checks-approved fully-signed orp-approved reconstruction-approved tests-approved urgent