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

Implementation of Dark Bremsstrahlung Custom Process #38329

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

revering
Copy link

PR description:

Implemented physics for dark bremsstrahlung and cross section biasing into SimG4Core/CustomPhysics. Overview of implementation method and motivation available from presentation to SIM group: https://indico.cern.ch/event/1162548/contributions/4882745/attachments/2447856/4194591/Revering_DarkBrem_Sim_22_5_20.pdf

Dark Bremsstrahlung is implemented as an optional process in the CustomPhysics module, and an example _cfi file with brief instructions has been added to SimG4Core/CustomPhysics/DarkBrem_SIM_cfi.py. The new process should have no impact on event generation that does not explicitly call it and include the necessary watcher plugin. A location dependent cross section biasing class is also implemented, and as with the process itself is only instantiated through the DBremWatcher plugin.

PR validation:

A small sample of events was generated locally in CMSSW_12_5_X_2022-06-09-1100 to verify that the cross section biasing was taking place in the correct areas of the detector and was producing events at a reasonable rate. Large samples have been generated in CMSSW_10_2_6 to validate the process itself and verify that the cross section calculations are functional for both the total cross section and the expected kinematics. The kinematic calculations do not have any reliance on external cmssw modules, so the behavior should be identical between the two versions.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38329/30508

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @revering for master.

It involves the following packages:

  • SimG4Core/CustomPhysics (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @cvuosalo, @rovere, @bsunanda, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

G4ParticleDefinition* muonplus = G4MuonPlus::MuonPlusDefinition();
G4ProcessManager* pmplus = muonplus->GetProcessManager();
G4ProcessManager* pmminus = muonminus->GetProcessManager();
pmplus->AddProcess(new G4muDarkBremsstrahlung(mgfile), -1, 1, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

@revering , I would advice (-1, 1, 1) substitute by (-1, 6, 6)

@civanch
Copy link
Contributor

civanch commented Jun 11, 2022

@revering , sorry, I was not checking your proposal earlier. Now several comments. First of all technical:

  1. all source files should be *.cc
  2. all lines with std::cout should be removed or substituted by edm::LogVerbatim
  3. edm::LogInfo better to substitute by edm::LogVerbatim
  4. if a header file is not used in the header it should be removed or moved to the source
  5. #include "G4PhysicalConstants.hh" and #include "G4SystemOfUnits.hh" are allowed only in source files, not in headers.
  6. if you use "delete" keyword in a header the method should be public
  7. Watchers should be in src not inside plugin

Now more complicate comments, I am not 100% sure what is the best solution.

  1. If you implement some extra product in SimProducer, then you need to take into account that watchers and producers are MT only for Run3 in recent CMSSW releases, for Run-2 it will work only in 1 thread runs. If for your analysis the producer is needed strongly it is possible to implement extra products in this way, but you should take care to submit 1 thread job for Run-2 production.

  2. biasing approach is relatively new for Geant4 . In 10.4 and 10.7 the code may be different, I am not sure if it is MT safe in previous version. Are you sure that it is the only solution you may use? I would recommend to use simplest approach - use factor, defined in python configuration file as the particle mass. Multiply cross section by this factor - it is a transparent method.

  3. I do not fill numbers, what is the spectra of dark matter bremsstrahlung?

  4. May it be that this physics does not need G4VEnergyLossProcess interface by G4VEmProcess interface. The energy loss is used for simulation of ionisation - energy transfer to low-energy electrons. Here we have dark

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38329/30576

  • This PR adds an extra 28KB to repository

  • Found files with invalid states:

    • SimG4Core/CustomPhysics/src/DarkBremXsecBiasingOperator.cxx:
    • SimG4Core/CustomPhysics/src/XsecBiasingOperator.cxx:
    • SimG4Core/CustomPhysics/plugins/DBremWatcher.cc:

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38329/30577

  • This PR adds an extra 28KB to repository

  • Found files with invalid states:

    • SimG4Core/CustomPhysics/src/DarkBremXsecBiasingOperator.cxx:
    • SimG4Core/CustomPhysics/src/XsecBiasingOperator.cxx:
    • SimG4Core/CustomPhysics/plugins/DBremWatcher.cc:

@cmsbuild
Copy link
Contributor

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

@revering
Copy link
Author

@civanch Thank you for the comments! I've modified the implementation to hopefully fix the technical issues detailed in notes 1-7, and changed the process to inherit from G4VEmProcess instead of G4VEnergyLossProcess as suggested in note 4. I produced a small sample of 100 events, and the process still occurs around the expected rates.

The main issue remaining is the question of the biasing and multi-threading. While we certainly could implement a global multiplicative factor as you suggest, we would need to re-implement the location dependence and would lose the track weights produced by the biasing operator, limiting the bias factors we could apply without producing non-physical modification to the event locations. I'm not really sure how we could put the location dependence in - my best idea for now would be to modify SampleSecondaries() in the model so that it gets the detector location when an interaction is proposed, and then chooses whether or not to simulate the interaction depending on a given list of allowed detector regions.
I would really prefer to use the existing bias method if possible - we need the sim-level information about the process that is produced with DBremWatcher for our analysis, so we're already limited to single-threaded running in run 2 generation and we've seen that the biasing works alright there. Is there some kind of test we could run to see if the biasing works in multithreaded generation for run 3?

In addition to the concerns about multithreading and the bias factors, I'm worried about the process disabling that we do to limit the interaction and multithreading. When SampleSecondaries() is called in G4muDarkBremsstrahlungModel, it gets the process manager and sets the process activation to false. At the start of the next event, DBremWatcher then sets it back to true. Do you know if this will work with multithreading? From what I can tell the processManager is thread-local, but I'm not sure if the activation tables they use are a shared variable or not. If they are shared, then one thread producing a brem might turn off the process for the others, or a thread starting an event could turn it on for a thread that needs it disabled.

@civanch
Copy link
Contributor

civanch commented Jun 16, 2022

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f56f2d/25571/summary.html
COMMIT: d117c9b
CMSSW: CMSSW_12_5_X_2022-06-16-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38329/25571/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3659074
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3659050
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38329/31264

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f56f2d/26448/summary.html
COMMIT: d44f06d
CMSSW: CMSSW_12_5_X_2022-07-25-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38329/26448/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3667670
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3667646
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 210 log files, 47 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Jul 26, 2022

+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. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@civanch
Copy link
Contributor

civanch commented Jul 26, 2022

@revering , let us make a backport to 12_4 (it may be run3 MC production release). This backport is straightforward - the same files as in 12_5 may be committed.

For 10_6 (Run2 MC production) the backport should be done with care, because modified classes may be different from those of 12_5. It is not possible blindly substitute files but it is needed check classes one by one.

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d274e60 into cms-sw:master Jul 26, 2022
@@ -0,0 +1,40 @@
#ifndef SIMAPPLICATION_APRIMEPHYSICS_H_
#define SIMAPPLICATION_APRIMEPHYSICS_H 1_
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible multiple include here (SIMAPPLICATION_APRIMEPHYSICS_H_ vs. SIMAPPLICATION_APRIMEPHYSICS_H)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, thank you @forthommel !
I submitted a fix in #38862

@aandvalenzuela
Copy link
Contributor

Hello,

It seems this PR broke G4VECGEOM IBs #38935.
Could you please have a look at it?

Many thanks,
Andrea.


G4bool IsApplicable(const G4ParticleDefinition& p) override;

void PrintInfo() override;
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, this member function could be removed (and that would fix the G4VECGEOM IB).

Copy link
Author

Choose a reason for hiding this comment

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

I've made a new PR #38949 with PrintInfo() removed to resolve the issue.

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.

8 participants