-
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
[DD4hep] Fix DDFilteredView unit test for DD4hep with Geant4 units #32721
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32721/20857
|
A new Pull Request was created by @cvuosalo (Carl Vuosalo) for master. It involves the following packages: DetectorDescription/DDCMS @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
type bug-fix |
@ianna FYI |
@mrodozov This PR fixes the first unit test failure in cmsdist PR 6554. |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-044d90/12489/summary.html Comparison SummarySummary:
|
+1 |
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 |
With the switch to DD4hep using the Geant4 units convention, a problem in a
DDFilteredView
unit test was revealed. An earlier change meant that numeric SpecPars with "eval=true" are always evaluated. There was old code in this unit test that assumed unevaluated string values of numerics could be fetched, and this code happened to work as long as the ROOT units convention was in effect. With the units convention change, this code no longer worked and needed a fix.PR validation:
The code was changed to be invariant under units change, so it should work with the new units convention. But my attempts to test with the cmsdist PR were not successful, so that PR will instead need to be tested against this one.
No backport is needed.