Validation/Geometry: No histos ROOT file being created by configuration scripts && CleanUp [Backport] #25139
Conversation
In the past, the statement to save the histograms in a ROOT file was written in the destructors of some classes, with the later use of Smart Pointers the ROOT file was not being written. endOfRun(), which should be called explicitely from MaterialBudgetAction includes a call to TestHistoMgr::save().
Replacing std::cout for edm::Log* methods to clean up output.
|
A new Pull Request was created by @vargasa (Andres Vargas) for CMSSW_10_3_X. It involves the following packages: Validation/Geometry @andrius-k, @Dr15Jones, @kmaeshima, @cvuosalo, @schneiml, @ianna, @mdhildreth, @cmsbuild, @jfernan2, @civanch can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Hi @vargasa, the changes seem to be different between this PR and a forward-port. Can you confirm that this is because of the different bases? |
For details, please check review: cms-sw#24625 (review)
For detais, please check review: cms-sw#24625 (review)
|
Pull request #25139 was updated. @andrius-k, @Dr15Jones, @kmaeshima, @cvuosalo, @schneiml, @ianna, @mdhildreth, @cmsbuild, @jfernan2, @civanch can you please check and sign again. |
|
@andrius-k I added the rest of the commits. From #24625 the latest commits were added as requested to change in the context, not in the changes itself. Please let me know if you see any differences after this |
|
backport of #24625 |
|
please test @vargasa, now diffs seem to be the same, thanks. |
|
The tests are being triggered in jenkins. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
@vargasa for which specific task is the backport of this PR to 10_3_X needed? It would naively look a tool for future developments |
| theID=0; | ||
| thePt=0; | ||
| theEnergy=0; | ||
| theMass=0; | ||
|
|
||
| theSupportMB = 0.; |
There was a problem hiding this comment.
@vargasa - to avoid a conversion from double to float, please use 0.f
theSupportMB is float, 0. is double:
https://en.cppreference.com/w/cpp/language/floating_literal
| @@ -20,217 +16,39 @@ MaterialBudgetData::MaterialBudgetData() | |||
|
|
|||
| MaterialBudgetData::~MaterialBudgetData() { | |||
There was a problem hiding this comment.
MaterialBudgetData::~MaterialBudgetData() = default;
| @@ -369,8 +226,10 @@ void MaterialBudgetData::dataPerStep( const G4Step* aStep ) | |||
| theOtherFractionMB = myMaterialBudgetCategorizer->x0fraction(materialName)[5]; | |||
| theAirFractionMB = myMaterialBudgetCategorizer->x0fraction(materialName)[6]; | |||
|
|
|||
| if(theOtherFractionMB!=0) std::cout << " material found with no category " << materialName | |||
| << " in volume " << volumeName << std::endl; | |||
| if(theOtherFractionMB!=0) | |||
There was a problem hiding this comment.
theOtherFractionMB is float, 0 is an integer
|
+1 |
cmsbuild
added
fully-signed
geometry-approved
and removed
geometry-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_10_3_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_10_4_X is complete. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
@fabiocos Please check #23770 (comment) With no ROOT files being produced the module is useless :( |
Perhaps @mmusich can address this question |
|
@fabiocos, we are planning to revamp material budget validation as part of release validation, hence needs to be functional in all open release cycles |
|
@mmusich "all open release cycles" includes back to 7_1_X and 5_3_X, I doubt you mean this. "Open development cycle" (where BTW next upgrade activities are planned) is 10_4_X. 10_3_X is for HI data taking and MC, possibly B-parking (although I haven't seen anything about this so far). Not sure which material budget studies should be done here, may be you have a plan about this and could clarify. Although merging this PR in 10_3_X should be harmless, I would like to get a logic and a plan, backporting whatever wherever without a clear target is not a good practice in my view. |
@fabiocos this is not random. As explained in the original PR #24625 another previously merged PR #23770 introduced a bug #23770 (comment) which is being fixed by #24625 and being backported here. Why 10_3_X? because this is the earliest release that includes the commit (as you can check here) producing the bug (which is the absence of these ROOT files, which are required for any analysis using Validation/Geometry package) . As you pointed out it is harmless to merge it but it is harmful not to do it as the release will feature a bug which has already been fixed in the master branch #25072. Please keep in mind that this is not feature development but bug fixing (despite the fact that includes some 'cleanup' commits and output redirection). This comment also applies to the other submitted backport #25143 which has not been merged |
|
+1 |
Backport of #24625
For details please check: