-
Notifications
You must be signed in to change notification settings - Fork 207
Update Vecgeom to v1.2.10 #9675
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
Conversation
|
please test |
|
A new Pull Request was created by @iarspider for branch IB/CMSSW_15_0_X/master. @iarspider, @smuzaffar can you please review it and eventually sign? Thanks. |
|
cms-bot internal usage |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c9b420/44185/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison SummarySummary:
|
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c9b420/44200/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
|
@smuzaffar , one of the main improvements in 1.2.10 are fixes in Polycone. CMS geometry has many envelops described as Polycones. This may explain why simulation history is changed even to tests with small statistics. in the G4VECGEOM branch we have tested this version of VecGeom. So, it is possible to accept this PR but we should trying to control validation results with 15_0 deployment. |
|
+externals @cms-sw/orp-l2 , this PR backports Vecgeom 1.2.10 from G4VECGEOM IBs. @cms-sw/simulation-l2 has tested it in |
|
This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_15_0_X/master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @mandrenguyen, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
@cms-sw/orp-l2 , we (SIM) would have really liked this to go in pre3.... |
|
@cms-sw/simulation-l2 , this did not go in the 15.0.0.pre3. I am afraid it is now late for this to go in 15.0.0 release. We can integrate it for 15.1.X now. |
|
It is quite unfortunate to have to wait another month for this to be in a prerelease. But please merge it as soon as possible. |
|
I would think that @smuzaffar is correct, let us do not break software process in this case. After CMSSW_15_1_0_pre1 we may return back to discussion if this version should be included in 15_0 or not. |
|
lets get this in 15.1.X IBs |
|
With a good motivation we could theoretically build a pre-release on 15_1_X and just skip it for release validation. |
|
The primary reason we wanted this in 15_0_X was to have a stable base for Celeritas development. I'm not sure if that is sufficient to build an early 15_1 prerelease. |
|
I believe we should not break data taking even if probability that this VecGeom version has problems is very small. Celeritas development should be on top of whatever CMSSW, for example, the current master. For stability any pre-release may be used. If it is urgent, let us ask for earlier pre1. To me it is not critical, pre1 will appears in any case soon. |
I don't see that this was requested in the spreadsheet, as I asked here: A github notification two days after the pre-release deadline is probably not the best way to make sure that a PR doesn't slip through the cracks. |
|
+1 |
resolves #9671