[DD4hep] Fix PPS simulation so physical volume names are recognized #32667
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32667/20743
|
|
A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master. It involves the following packages: SimG4CMS/PPS @cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Thanks @cvuosalo ! I wonder whether this problem would occur also at other places where volume names are used, e.g.: |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c06f50/12277/summary.html Comparison SummarySummary:
|
|
do any of these codes get run in the event loop? |
|
+1 I would guess none is run inside the event loop |
cmsbuild
added
fully-signed
simulation-approved
and removed
pending-signatures
simulation-pending
labels
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
@jan-kaspar @ghugo83 I think those places in |
|
@davidlange6 @civanch The code changed in this PR is run during event simulation when there are hits in the corresponding PPS detectors. Simulation has to generate several events before one happens to create hits in the PPS detectors, so one can run DD4hep simulation on a few events without seeing the bugs fixed in this PR. |
|
My question was are we now doing regexp searches in the event loop?
… On Jan 14, 2021, at 5:53 PM, Carl Vuosalo ***@***.***> wrote:
@davidlange6 @civanch The code changed in this PR is run during event simulation when there are hits in the corresponding PPS detectors. Simulation has to generate several events before one happens to create hits in the PPS detectors, so one can run DD4hep simulation on a few events without seeing the bugs fixed in this PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@davidlange6 If the |
|
ouch. Thanks for the pointer.. but I guess not many tracks hit these detectors so there aren’t many steps through the sensitive detectors...
… On Jan 14, 2021, at 6:54 PM, Carl Vuosalo ***@***.***> wrote:
@davidlange6 SimG4CMS/PPS/src/PPSPixelOrganization.cc (line 42) already was using the contains string matching method prior to this PR. This string matching is not done for every event (otherwise the bug would have terminated simulation at event 1). It is only done for events that have hits in the PPS detectors.
If the contains method should not be used at all in simulation because of the performance cost, then that would be a task for the PPS simulation developers ***@***.*** @mundim) to optimize the code for better performance.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@ghugo83 Not only does the namespace have to be stripped off the physical volume name, but also the copy number, so that's why I used |
|
@cvuosalo Ok makes sense. |
|
On Thu, 14 Jan 2021 at 20:04, ghugo83 ***@***.***> wrote:
@jan-kaspar <https://github.com/jan-kaspar> @ghugo83
<https://github.com/ghugo83> I think those places in DetGeomDesc use the
logical volume name that has already had the namespace stripped off, but it
would be good if you would double-check that those lines of code are
working correctly with DD4hep.
Yes these were cross-checked and fully validated (at least with the XMLs
available at the time).
Great, thanks!
|
Hi, Sorry for the delay, trying to catch up after vacations... So, this piece was introduced by the original developer (@fabferro ) or by the former ones because in some PPS sensors, due to the way the sensitive volumes where designed, there were hits almost everywhere in the PPS region. If this is a problem, I'm afraid we will have to go deeper in debugging the geometry. |
Oh.. ok. thanks Carl. |
|
@mundim I think the conclusion is that string comparisons should be minimized in the simulation code, especially in loops and other sections that are executed many times. If they are essential, then they have to be kept. PPS developers can evaluate whether any changes need to be made. |
In old DD, physical volume names have no prefix or suffix, but, with DD4hep, physical volume names are prefixed with the namespace and suffixed with the copy number. PPS code searches for specific physical volumes, and these searches were failing because of the additional prefix and suffix added by DD4hep. The solution is to search for a string that somewhere contains the base physical volume name, rather than requiring an exact match.
This PR fixes issue #32653. The exception described in the issue occurred because the intended physical volume was not found, resulting in an attempt to construct a nonsense DetId, which caused the exception. In addition to fixing the search for the volume, this PR changes the initialization of the data members used to create the DetId. These members had been initialized to -1, which was a poor choice since they were converted into unsigned ints for use in the DetId, which gave the maximum unsigned value instead of -1 and thus generated an exception. This PR changes the initialized values to 0, which will not generate an exception in the case that the physical volume cannot be found. Instead, this PR adds error messages to cover this case.
PR validation:
The sim step of DD4hep workflow 11642.911 was run to see the bug occur. After the code was fixed, the sim step was run again to see that it could successfully handle 50 events without the error occurring. Also, debug statements were checked to see that the fix was working.
No backport is planned.