[Do not merge] Update classversion CLHEP::Hep3Vector #32420
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32420/20286
|
|
A new Pull Request was created by @mrodozov (Mircho Rodozov) for master. It involves the following packages: DataFormats/CLHEP @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test with cms-sw/cmsdist#6486 |
|
-1 CMSSW: CMSSW_11_3_X_2020-12-08-1100 I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test testTauEmbeddingProducers had ERRORS |
|
Comparison results are now available Comparison Summary:
|
|
please test with cms-sw/cmsdist#6492 |
|
scary…
But you need to keep the checksum for class version 10 in this dictionary.
… On Dec 9, 2020, at 1:08 PM, Mircho Rodozov ***@***.***> wrote:
please test with cms-sw/cmsdist#6492
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
should it be one additional record like: |
|
abort |
|
I see there are other records like this in the same file |
|
No, like this:
<class name="CLHEP::Hep3Vector" ClassVersion="11">
<version ClassVersion="11" checksum="newchecksum"/>
<version ClassVersion="10" checksum="3040806103"/>
</class>
… On Dec 9, 2020, at 2:05 PM, Mircho Rodozov ***@***.***> wrote:
should it be one additional record like:
<class name="CLHEP::Hep3Vector" ClassVersion="11">
<version ClassVersion="11" checksum="newchecksum"/>
</class>
<class name="CLHEP::Hep3Vector" ClassVersion="10">
<version ClassVersion="10" checksum="3040806103"/>
</class>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@mrodozov , if I understand correctly, today we cannot simply include new CLHEP in 11_3_GEANT4_X, because it is another external? In 11_3_GEANT4_X we take master and can add only Geant4 modification, not other external libraries? |
|
Which of reported errors do you believe are connected to CLHEP?
… On Dec 10, 2020, at 1:27 PM, Vladimir Ivantchenko ***@***.***> wrote:
@mrodozov , if I understand correctly, today we cannot simply include new CLHEP in 11_3_GEANT4_X, because it is another external? In 11_3_GEANT4_X we take master and can add only Geant4 modification, not other external libraries?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
the unit test failing looks unrelated, but in case we wanted to get this I would've tried to figure it out where this python failure comes from. we can have only one version of |
|
Hum,
is this the right set of diffs ?
cms-externals/clhep@cms/v2.4.1.3...cms/v2.4.4.0
Quite odd that something here triggered a Hep3Vector version change
… On Dec 10, 2020, at 3:12 PM, Mircho Rodozov ***@***.***> wrote:
the unit test failing looks unrelated, but in case we wanted to get this I would've tried to figure it out where this python failure comes from.
@civanch almost, yes, we can add new externals to GEANT4 as we changed VecGeom last week - as long as the externals does not require changes like this one in CMSSW. Now, CLHEP requires changes also in cmssw
we can have only one version of
<class name="CLHEP::Hep3Vector" ClassVersion="11">
in CMSSW master
but
geant4 cmsdist branch with clhep 2440 will require version 11
master cmsdist branch with clnep 2413 will require version 10
and because we have only one branch on cmssw that we use for both, we must have another one for geant4
@smuzaffar should we branch master with another name to use for geant4 ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
there is no commit history because I took clhep 2.4.4.0 from a tar file following: |
|
I may be do not understand something, but in the diff between new branch v2.4.4.0 and the master I do not see any difference in numerical constants, while in the CLHEP documentation there is an entry: 2020-07-20 Gabriele Cosmo Gabriele.Cosmo@cern.ch
This is a claim, that they peaked up from Britannica and Wikipedia new values for number of constants. Values are changed on level of 5th or 6th digits, which should change slightly cross sections and secondary distributions on such level, likely changing simulation histories. By the way, I cannot see this in our repository, only in CLHEP original. |
|
Strange. From what I see
git clone -b cms/v2.4.4.0 git@github.com:cms-externals/clhep.git
git clone -b CLHEP_2_4_4_0 https://gitlab.cern.ch/CLHEP/CLHEP.git gitlab
diff -r clhep/ gitlab/
Seems correct.
|
|
It seems something is wrong with the diffs link (not sure what.. anyone see why?) https://gitlab.cern.ch/CLHEP/CLHEP/-/compare/CLHEP_2_4_1_3...CLHEP_2_4_4_0 |
|
and the Hep3Vector change |
-static constexpr double h_Planck = 6.62606896e-34 * joules; -static constexpr double k_Boltzmann = 8.617343e-11 * MeV/kelvin; -static constexpr double Avogadro = 6.02214179e+23/mole; |
|
On Dec 10, 2020, at 3:12 PM, Mircho Rodozov ***@***.***> wrote:
the unit test failing looks unrelated, but in case we wanted to get this I would've tried to figure it out where this python failure comes from.
@civanch almost, yes, we can add new externals to GEANT4 as we changed VecGeom last week - as long as the externals does not require changes like this one in CMSSW. Now, CLHEP requires changes also in cmssw
we can have only one version of
<class name="CLHEP::Hep3Vector" ClassVersion="11">
in CMSSW master
but
geant4 cmsdist branch with clhep 2440 will require version 11
master cmsdist branch with clnep 2413 will require version 10
and because we have only one branch on cmssw that we use for both, we must have another one for geant4
@smuzaffar should we branch master with another name to use for geant4 ?
One option you have is to pick up a simple patch via cmssw-queue-override.file
|
|
one problem with cmssw-queue-override.file is that it is not used by PR testing setup or for local development (e.g. cms-addpkg is not going to apply these patches) |
|
Again, I may be not understand something but He3Vector change is the optimization of the code itself: https://gitlab.cern.ch/CLHEP/CLHEP/-/commit/5f20daf0cae911793d9aa5fc62987d6dc83647ab |
|
We may request PPD validation of new CLHEP+VecGeom on top of existing master together with 11_3_0_pre1 or 11_2_0. |
|
@mrodozov , @smuzaffar , how we can resolve this problem? I see two possibilities: 1) ask PPD validation of the new CLHEP separate, later we will ask for validation of GEANT4 10.7; 2) asking validation of CLHEP+Geant4 10.7 together? |
|
@civanch CLHEP validation only will be more simple because you have to test Geant 10.7 for some time and ask for validation later on |
|
On Dec 10, 2020, at 5:32 PM, Vladimir Ivantchenko ***@***.***> wrote:
Again, I may be not understand something but He3Vector change is the optimization of the code itself: https://gitlab.cern.ch/CLHEP/CLHEP/-/commit/5f20daf0cae911793d9aa5fc62987d6dc83647ab
but not change any functionality of the class.
The data members of the Hep3Vector class changed… So as usual, to store it persistently the checksum needs to be updated accordingly..
|
|
@civanch , @davidlange6 I have created CMSSW_11_3_GEANT4_X branch. As discussed during ORP, we can include new clhep and geant4 (along with this change here) for geant4 IBs and ask for a validation. |
|
please test with cms-sw/cmsdist#6486 |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-36750b/11699/summary.html Found compilation warnings Comparison SummarySummary:
|
|
looks good for geant4 IB. lets get this merged for geant4 IBs so that we can build g4 release for validation |
|
please test with cms-sw/cmsdist#6512 |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-36750b/11746/summary.html Found compilation warnings Comparison SummarySummary:
|
PR description:
This is needed for testing geant4 changes, since clhep is updated
We cannot have this in master, not until we use the updated version of clhep for our master on https://github.com/cms-sw/cmsdist
PR validation:
DataFormats/CLHEP is building