Rpc digi dev v9#47447
Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47447/43859 Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47447/43875 |
|
A new Pull Request was created by @bapavlov for master. It involves the following packages:
The following packages do not have a category, yet: DataFormats/IRPCDigi @Dr15Jones, @civanch, @cmsbuild, @kpedro88, @makortel, @mdhildreth, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@bapavlov some initial questions/comments:
|
|
I have no strong opinion about the new package either way. To me, it is up to the DPG to decide how they want to organize their code at this level. That said, my detailed code review is waiting on responses to the initial questions above. |
|
@bapavlov just to be clear, I am waiting on your replies to #47447 (comment) before further review or testing. |
cmsbuild
added
code-checks-pending
and removed
new-package-pending
code-checks-approved
labels
|
@kpedro88, @civanch We discussed your comments #47447 (comment) within the RPC group and decided to follow Vladimir's suggestion to move IRPCDigi to the DataFormats/RPCDigi directory. I assume that, in this case, there is no need of a companion PR to https://github.com/cms-sw/cms-bot. Regarding your question about the workflow: currently, only the DIGI step for RPCPhase2 and IRPC is implemented, and no dedicated workflow is included yet. I have a technical question about how to proceed with implementing the workflow.
Your guidance on this would be greatly appreciated. |
|
@bapavlov indeed, if there is no new package, then no PR to cms-bot is needed. For now, you should create a new Modifier to enable this change in the DIGI step. Then you can add a special workflow, as documented at https://github.com/cms-sw/cmssw/tree/master/Configuration/PyReleaseValidation. You can follow the |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47447/43939
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47447/49589
|
|
Pull request #47447 was updated. @AdrianoDee, @Alejandro1400, @DickyChant, @Dr15Jones, @JanChyczynski, @Martin-Grunewald, @Moanwar, @alja, @antoniovagnerini, @arunhep, @atpathak, @bsunanda, @civanch, @cmsbuild, @ctarricone, @davidlange6, @emeschi, @fabiocos, @francescobrivio, @ftenchini, @gabrielmscampos, @jfernan2, @kfjack, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @miquork, @mmusich, @perrotta, @rseidita, @smorovic, @smuzaffar, @srimanob, @sroychow, @ssekmen, @tvami can you please check and sign again. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47447/49590
|
|
Pull request #47447 was updated. @AdrianoDee, @DickyChant, @Dr15Jones, @antoniovagnerini, @civanch, @cmsbuild, @davidlange6, @fabiocos, @ftenchini, @kfjack, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @miquork, @smuzaffar, @sroychow can you please check and sign again. |
|
@kpedro88 I removed some commits to fixed the rebase. The only thing that I added after the rebase are:
Yesterday cmsbuild reported a conflict, so I also added:
|
|
There are still unrelated commits in this branch (see the "commits" tab in this PR). The recent use of I have made a version of the branch reset to before (where the commit hash is the actual first commit in this branch, "Squashed commit of the following: ..."), the ^ means the first parent of that commit, and |
commit 4f66246 Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Mon Mar 3 21:11:40 2025 +0100 IRPCDigi to DataFormats/RPCDigi directory commit 2c99471 Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Tue Feb 25 23:16:12 2025 +0100 applying code-checks patch commit 2e6e9d4 Merge: 26ffcf1 b2dc087 Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Tue Feb 25 23:04:58 2025 +0100 Merged rpc_digi_dev_v9 from repository bapavlov with cms-merge-topic commit b2dc087 Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Wed Feb 5 13:53:59 2025 +0100 adding changes to the confriguration files commit fd5c136 Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Wed Feb 5 13:51:51 2025 +0100 adding the digitization code commit 7b32d20 Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Wed Feb 5 13:47:32 2025 +0100 adding the new IRPCDigi format commit e6f63fa Author: Borislav Pavlov <Borislav.Pavlov@cern.ch> Date: Wed Feb 5 13:46:19 2025 +0100 adding RPCDigiPhase2 format
Co-authored-by: Matti Kortelainen <matti.kortelainen@cern.ch>
Co-authored-by: Matti Kortelainen <matti.kortelainen@cern.ch>
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47447/49607
|
|
@kpedro88 Thank you very much !!! I followed your instructions - the code compiles and runs successfully with runTheMatrix.py -l 34434.62 -w upgrade |
|
Pull request #47447 was updated. @AdrianoDee, @DickyChant, @Dr15Jones, @antoniovagnerini, @civanch, @cmsbuild, @davidlange6, @fabiocos, @ftenchini, @kfjack, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @miquork, @smuzaffar, @sroychow can you please check and sign again. |
|
please test |
| @@ -0,0 +1,44 @@ | |||
| #ifndef IRPCDigi_IRPCDigi_h | |||
There was a problem hiding this comment.
Please follow code rule 4.1: this should be #ifndef DataFormats_RPCDigi_IRPCDigi_h (& similar for all other header files in this PR)
| if (strip_ != digi.strip() || bxLR_ != digi.bx()) | ||
| return false; | ||
| return true; |
There was a problem hiding this comment.
simplify:
return strip_ == digi.strip() && bxLR_ = digi.bx();| if (strip_ != digi.strip() || bx_ != digi.bx()) | ||
| return false; | ||
| return true; |
|
|
||
| float IRPCDigiTime::coordinateY() { | ||
| const double signal_speed = 0.66 * 299792458e-7; //signal propagation speed [cm/ns] | ||
| return signal_speed * (timeLR() - timeHR()) / 2.; |
There was a problem hiding this comment.
return signal_speed * time(); to avoid duplication
| float IRPCDigiTime::time() { return (timeLR() + timeHR()) / 2.; } | ||
|
|
||
| float IRPCDigiTime::coordinateY() { | ||
| const double signal_speed = 0.66 * 299792458e-7; //signal propagation speed [cm/ns] |
There was a problem hiding this comment.
in principle, physical constants like speed of light should be taken from a central source, but I'm not sure if e.g. CLHEP usage is recommended in DataFormats
There was a problem hiding this comment.
CLHEP dependence is allowed in DataFormats (we even have DataFormats/CLHEP).
| @@ -0,0 +1,267 @@ | |||
| #include "Geometry/RPCGeometry/interface/RPCRoll.h" | |||
There was a problem hiding this comment.
propagate all comments on IRPCSimModelTiming.cc to this very similar file/class
There was a problem hiding this comment.
do these really need to be two separate classes duplicating hundreds of lines of code?
| double aveEff; | ||
| double aveCls; | ||
| double resRPC; | ||
| double timOff; | ||
| double dtimCs; | ||
| double resEle; | ||
| double sspeed; | ||
| double lbGate; | ||
| bool rpcdigiprint; | ||
| bool eledig; | ||
|
|
||
| int N_hits; | ||
| int nbxing; | ||
| double rate; | ||
| double gate; | ||
| double frate; | ||
| bool do_Y; | ||
| double sigmaY; | ||
|
|
||
| std::map<int, std::vector<double> > clsMap; | ||
| std::vector<double> sum_clsize; | ||
| std::vector<double> clsForDetId; | ||
| std::ifstream* infile; | ||
|
|
||
| RPCSynchronizer* _rpcSync; |
|
|
||
| float prop_time = distanceFromEdge / sspeed; | ||
|
|
||
| // double rr_tim1 = CLHEP::RandGaussQ::shoot(engine, 0.,resRPC); |
| tdc.first = BX; | ||
| tdc.second = SBX; |
| std::pair<float, float> getDoubleTiming(const PSimHit* simhit, CLHEP::HepRandomEngine* engine, float StripLength); | ||
| int getBX(float time); | ||
| std::pair<int, int> getBX_SBX(float time); | ||
| //std::pair<int,int> getFineTime(const PSimHit* simhit, CLHEP::HepRandomEngine* engine,float StripLength); |
|
+1 Size: This PR adds an extra 20KB to repository Comparison SummarySummary:
|
|
Milestone for this pull request has been moved to CMSSW_20_0_X. Please open a backport if it should also go in to CMSSW_17_0_X. |
|
Just checking, is this PR intended for Phase 2 (20_1_X) or Run 3 legacy (17_0_X) or both? |
|
This is a Phase 2 development. |
|
Thanks, then no further action is needed regarding the release cycle change. |
11 participants
PR description:
We would like to introduce some new code for the RPC Phase 2 upgrade.
So far, the digitization for both the currently installed RPCs and the ones planned for the Phase 2 upgrade have used the same data format, which was developed for the Muon upgrade TDR. However, this format is no longer adequate due to the changes planned for the RPC Phase 2 upgrade.
In short, the RPC Phase 2 upgrade includes two major changes:
We would like to introduce these changes into the software as well. We will keep the current digi format intact, but we plan to add two additional digi formats, called RPCDigiPhase2 and IRPCDigi. Below is a brief description of the proposed changes:
PR validation:
At the moment, our colleagues are using the code from my repository and are developing the code for the next steps (e.g., trigger emulators). The code is tested and works as expected. Results have been reported during several RPC DPG and RPC Trigger meetings:
RPC DPG May 8, 2024
RPC Trigger Oct 2, 2024
RPC DPG Jan 22, 2025
RPC DPG Feb 5, 2025
I would like to emphasize again that we have taken special care not to interfere with the present simulation in any way.