Skip to content
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

Sensitive detectors 6 and watchers cleanup #23595

Merged
merged 14 commits into from Jun 18, 2018

Conversation

civanch
Copy link
Contributor

@civanch civanch commented Jun 15, 2018

This PR includes pure technical clean-up of SIM packages:

  1. MC truth related classes moved from SimG4Core/Application to SimG4Core/Notification
  2. Corresponding dependences and headers in SimG4CMS and SimG4Core updated
  3. Protection for usage of watchers is removed allowing watchers to be used in MT mode
  4. Const pointers to Geant4 objects are used where possible
  5. unused or empty methods removed
  6. empty and commented lines removed

Correctness of this PR means no difference versus baseline.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@civanch
Copy link
Contributor Author

civanch commented Jun 15, 2018

PR is not yet fully completed but the test will be started in order to see is something is forgotten.

@civanch
Copy link
Contributor Author

civanch commented Jun 15, 2018

please test

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 15, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28709/console Started: 2018/06/15 15:10

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @civanch (Vladimir Ivantchenko) for master.

It involves the following packages:

SimG4CMS/Calo
SimG4CMS/FP420
SimG4CMS/Forward
SimG4CMS/Muon
SimG4CMS/ShowerLibraryProducer
SimG4CMS/Tracker
SimG4Core/Application
SimG4Core/GeometryProducer
SimG4Core/Notification

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor

@civanch

  1. Protection for usage of watchers is removed allowing watchers to be used in MT mode

Have the watchers been made thread safe? Or is the point to be able to run those which are thread safe, and leave the thread-safeness of watchers as the problem of the one who wants to run them?

@civanch
Copy link
Contributor Author

civanch commented Jun 15, 2018

@makortel , we have discussed at SIM meeting mainly with Sunanda, that we will update all watchers to be thread safe. A good part of old watchers are not revocable for MT but not all of them are used now, I have in mind removing them. We need removing the protection because in exotica sector there is a use case where watcher is needed for this production. First we restore exotica case, then will recover other useful watchers.

@cmsbuild
Copy link
Contributor

-1

Tested at: 9dc586d

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23595/28709/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found an error when building:

>> Entering Package SimG4Core/Application
Entering library rule at SimG4Core/Application
>> Compiling  /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/src/SimG4Core/Application/src/SteppingAction.cc 
In file included from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/src/SimG4Core/Application/interface/EventAction.h:6:0,
                 from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/src/SimG4Core/Application/src/SteppingAction.cc:3:
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/poison/SimG4Core/Application/interface/G4SimEvent.h:1:2: error: #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
 #error THIS FILE HAS BEEN REMOVED FROM THE PACKAGE.
  ^~~~~
>> Compiling  /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/src/SimG4Core/Application/src/ExceptionHandler.cc 
In file included from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/src/SimG4Core/Application/interface/EventAction.h:6:0,
                 from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-06-15-1100/src/SimG4Core/Application/src/SteppingAction.cc:3:


@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@civanch
Copy link
Contributor Author

civanch commented Jun 16, 2018

please test

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 16, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/28724/console Started: 2018/06/16 21:16

@cmsbuild
Copy link
Contributor

Pull request #23595 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23595/28724/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 2902004
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2901813
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 128 log files, 14 edm output root files, 31 DQM output files

@civanch
Copy link
Contributor Author

civanch commented Jun 17, 2018

+1

@cmsbuild
Copy link
Contributor

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)


// define real distances:
float d = fabs(tan(th)-tan(thFibDirRad));
float d = std::abs(tan(th)-tan(thFibDirRad));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, when is this next iteration planned? Not for 10_2_X I assume at this point

double hadr; //EnergyDeposit of HD particles
double theIncidentEnergy; //Energy of the primary particle
G4int theTrackID; //Identification number of the primary
float elem; //EnergyDeposit of EM particles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the move from double to float in this class supposed to leave the result unchanged? A priori I would imagine no. This class is used in the BHM and in the FastTimer/MTD sensitive detectors. The latter are PhaseII detectors for which I doubt we have histograms that could spot differences. Not a big dal but...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos , in the SD classes these values are float or int, so the same type should be members of this hit.

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 7549462 into cms-sw:master Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants