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

Fixed run time problem in hadron elastic process #68

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

civanch
Copy link
Contributor

@civanch civanch commented Jan 17, 2023

Two fixes are added, which are available in Geant4 master:

  1. fixed run time computations for hadron elastic process
  2. fixed warning on "unused variable" in G4CoupledTransportation

Both are useful for CMSSW.

@cmsbuild
Copy link

A new Pull Request was created by @civanch (Vladimir Ivantchenko) for branch cms/v11.1.0.

@cmsbuild, @smuzaffar, @aandvalenzuela, @iarspider can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.
cms-bot commands are listed here

@smuzaffar
Copy link
Contributor

please test for CMSSW_13_0_GEANT4_X

@civanch
Copy link
Contributor Author

civanch commented Jan 17, 2023

@smuzaffar , is it possible to test these fixes before the pre-release is out?

Removed two modifications which are not relevant to avoid compilation problem
@cmsbuild
Copy link

Pull request #68 was updated.

@smuzaffar
Copy link
Contributor

Only testing we can do is to build/run G4 IB, once PR tests are done then we can merge and build an IB for you to do the testing

@smuzaffar
Copy link
Contributor

please test for CMSSW_13_0_GEANT4_X

@civanch
Copy link
Contributor Author

civanch commented Jan 17, 2023

@smuzaffar , yes, it is clear. Unfortunately, I was too fast - one file should be redone and recommitted, because inside Geant4 branch there was also development, not only bug fix. I will try make it and update the PR.

@cmsbuild
Copy link

Pull request #68 was updated.

@cmsbuild
Copy link

Pull request #68 was updated.

@smuzaffar
Copy link
Contributor

@smuzaffar , yes, it is clear. Unfortunately, I was too fast - one file should be redone and recommitted, because inside Geant4 branch there was also development, not only bug fix. I will try make it and update the PR.

ok, feel free to start the tests (using please test for CMSSW_13_0_GEANT4_X comment ) once you are done with your changes

@civanch
Copy link
Contributor Author

civanch commented Jan 17, 2023

please test for CMSSW_13_0_GEANT4_X

@smuzaffar
Copy link
Contributor

@civanch , which PR tests are running, you can use /cvmfs/cms-ci.cern.ch/week0/cms-externals/geant4/68/30029/CMSSW_13_0_GEANT4_X_2023-01-15-2300 area to run local tests e.g. you can run the following to set the env

cd /cvmfs/cms-ci.cern.ch/week0/cms-externals/geant4/68/30029/CMSSW_13_0_GEANT4_X_2023-01-15-2300
cmsenv
cd /tmp
# run tests

@smuzaffar
Copy link
Contributor

@civanch , build and relvals look good, if this is OK then I would like to merge it and build an IB based on it now

@civanch
Copy link
Contributor Author

civanch commented Jan 17, 2023

@smuzaffar , would be great

@smuzaffar smuzaffar merged commit 6d6ce02 into cms-externals:cms/v11.1.0 Jan 17, 2023
smuzaffar added a commit to cms-sw/cmsdist that referenced this pull request Jan 17, 2023
This is for special GEANT4 Vecgeom IBs and changes has been tested via cms-externals/geant4#68
@cmsbuild
Copy link

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c9de80/30029/summary.html
COMMIT: 977cf77
CMSSW: CMSSW_13_0_GEANT4_X_2023-01-15-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-externals/geant4/68/30029/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 66694 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555538
  • DQMHistoTests: Total failures: 125521
  • DQMHistoTests: Total nulls: 315
  • DQMHistoTests: Total successes: 3429680
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.4270000000000005 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): 0.586 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 11834.0 ): 0.938 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 250202.181 ): 0.357 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 25202.0 ): -0.098 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 7.3 ): -6.873 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 8.0 ): 6.517 KiB SiStrip/MechanicalView
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: found differences in 16 / 47 workflows

@smuzaffar
Copy link
Contributor

@civanch , GEANT4, G4VECGEOM and LTOG$VECGEOM IBs are now available with this change. Please run your tests as we might have to build 13.0.0.pre3 today.

@civanch
Copy link
Contributor Author

civanch commented Jan 18, 2023

@smuzaffar , I have run simple tests for 13_0_0_G4VECGEOM_X_2023_01-17-23 - results as expected. 13_0_0_LTOG4VECGEOM_X_2023-01-17-2300 give similar results but much less CPU advantage than expected 3.5%. Of course, my tests are not ideal and I saw similar effects for other SIM improvements - performance of SIM does not want to be improved.

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.

3 participants