-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Added new ZDCHitReconstructor and ZDCRecAlgo to work with QIE10 Digis and Updated ZDCRecHit #45407
Conversation
cms-bot internal usage |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45407/40858
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
@matt2275 why not just fix the code checks? |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45407/40904 |
A new Pull Request was created by @matt2275 for master. It involves the following packages:
@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@matt2275 Please provide a recipe to test this code |
There is a recipe to test the code here in the readme: https://github.com/matt2275/ZDCRecHit-Associated-Files/tree/main. |
The starting point is not from merging this PR. Can you provide a succinct recipe in the PR description please? |
I'm not positive what you mean by starting point. In the ReadMe file linked. I don't assume the PR is merged. The instructions start with starting a new CMSSW_14_1_0 working directory. From there, the relevant packages are added and files changed in this PR are updated and cmssw is rebuilt. I'll add the instructions to PR description. I'm not sure the cms git command to download specific folders from a fork so I specify each file that needs to be downloaded from the Test_ZDCRecHit branch. Would git cms-merge-topic matt2275:Test_ZDCRecHit successfully merge the the Test_ZDCRecHit branch with CMSSW_14_1_0? If so, that would make the recipe significantly shorter. |
Hi @matt2275 The you'd see that it:
[1] to be always used to resolve packages dependencies (on the changes made), before compilation and running any (test) jobs. |
Hi @abdoulline , I checked the git cms-merge-topic method last night however, there was an error compiling with a validation fix. I've attached a different method that only brings the changed packages however, I was not able to fully check the recipe due to disk quota issues. I know the recipe here works since it has been used by multiple people but it involved retrieving the updated files via wget. |
@matt2275 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is in src instead of plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ZDCSimpleRecAlgo.cc is in src so I kept ZDCSimpleRecAlgo_Run3.cc in src as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the discussion on plugins vs src here:
https://cms-sw.github.io/cms_coding_rules.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the discussion on plugin vs src, ZDCSimpleRecAlgo_Run3 should be in the src directory as it used by ZDCHitReconstructor_Run3.cc which is the EDProducer for ZdcRecHits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And ZDCHitReconstructor_Run3.cc
?
Shouldn't that be in plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be in the plugins folder, there is no plugins directory in RecoLocalCalo/HCalRecProducers. I can create the plugins directory and put ZDCHitReconstructor_Run.cc/ .h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can create the plugins directory.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45407/41515 |
Pull request #45407 was updated. @antoniovilela, @davidlange6, @fabiocos, @mandrenguyen, @rappoccio can you please check and sign again. |
please abort |
please test |
-1 Failed Tests: UnitTests RelVals-INPUT Unit TestsI found 3 errors in the following unit tests: ---> test testPhase2PixelNtuple had ERRORS ---> test testPhase2SkimmedGeometry had ERRORS ---> test PrimaryVertex had ERRORS RelVals-INPUT
Expand to see more relval errors ...
Comparison SummarySummary:
|
👍 |
ignore tests-rejected with ib-failure |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (test failures were overridden). This pull request will be automatically merged. |
ASAN IB is getting heap buffer overflows in WF 141.901, 141.902, 142, 142.901 and 142.902 (so far).
|
Okay thanks for alerting us, we can take a look! |
@dan131riley I think I don't see the issue anymore in CMSSW_14_2_X_2024-09-04-2300 |
PR description:
This is a draft pull request for adding new ZdcHitReconstuctor_Run3 and ZdcSimpleRecAlgo_Run3 to process QIE10 digis for the ZDC.
Additionally, ZDCRecHit was updated.
This update is being tested on CMSSW_14_1_0_pre5
PR validation:
No Validations tests have been run yet
Build Recipe
Note that this job will crash if RPD digis are processed without a temporary fix to HcalZDCDetId which is referenced here
Currently RPD digis are skipped so no crashes should occur